Skip to content

PMM-7 Fix PMM-T2227 tarball upgrade version check #1137

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
travagliad wants to merge 3 commits into from
Closed

PMM-7 Fix PMM-T2227 tarball upgrade version check #1137

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4ad12ee
PMM-7 Fix PMM-T2227 tarball upgrade version check
cursoragent Aug 4, 2026
aa7b80a
PMM-7 Use pmm-admin entrypoint for CLIENT_IMAGE version
cursoragent Aug 4, 2026
387dc46
PMM-7 Resolve PMM_VERSION from CLIENT_IMAGE for FB builds
cursoragent Aug 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 41 additions & 17 deletions cli/tests/generic.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ const PGSQL_USER = 'postgres';
const PGSQL_PASSWORD = 'pass+this';
const ipPort = async () => ((await cli.exec('docker ps')).stdout.includes('pdpgsql_pmm_') ? '127.0.0.1:5432' : '127.0.0.1:5447');

const getClientImageVersion = (): string => {
const clientImage = process.env.CLIENT_IMAGE ?? 'perconalab/pmm-client:3-dev-latest';
return JSON.parse(
cli.execute(`docker run --rm --entrypoint pmm-admin ${clientImage} --version --json`).stdout,
).Version;
};

test.describe('PMM Client "Generic" CLI tests', { tag: '@generic' }, async () => {
test.beforeAll(async ({}) => {
const result = await cli.exec('docker ps | grep pdpgsql_pmm | awk \'{print $NF}\'');
Expand All @@ -17,10 +24,9 @@ test.describe('PMM Client "Generic" CLI tests', { tag: '@generic' }, async () =>

let PMM_VERSION = `${process.env.CLIENT_VERSION}`;
if (/latest-tarball|3-dev-latest|pmm3-rc|https:/.test(PMM_VERSION)) {
// TODO: refactor to use docker hub API to remove file-update dependency
// See: https://github.com/Percona-QA/package-testing/blob/master/playbooks/pmm2-client_integration_upgrade_custom_path.yml#L41
PMM_VERSION = cli.execute('curl -s https://raw.githubusercontent.com/Percona-Lab/pmm-submodules/v3/VERSION')
.stdout.trim();
PMM_VERSION = process.env.CLIENT_IMAGE
? getClientImageVersion()
: cli.execute('curl -s https://raw.githubusercontent.com/Percona-Lab/pmm-submodules/v3/VERSION').stdout.trim();
}

test('Verify pt summary for mysql mongodb and pgsql', async ({}) => {
Expand Down Expand Up @@ -570,34 +576,52 @@ test.describe('PMM Client "Generic" CLI tests', { tag: '@generic' }, async () =>

test('PMM-T2227 - Verify tarball upgrade @generic', async ({}) => {
const containerName = 'tarball_client';
const tarballURL = process.env.PMM_CLIENT_VERSION?.includes('http')
? process.env.PMM_CLIENT_VERSION
: 'https://pmm-build-cache.s3.us-east-2.amazonaws.com/PR-BUILDS/pmm-client/pmm-client-latest.tar.gz';
const expectedUpgradeVersion = getClientImageVersion();

await cli.exec('docker network create pmm-qa || true');
await cli.exec('docker network connect pmm-server pmm-qa');
await cli.exec('docker network connect pmm-qa pmm-server || true');
await cli.exec(`docker rm -f ${containerName} 2>/dev/null || true`);
await cli.exec(`docker run --rm -d --name="${containerName}" --network="pmm-qa" --privileged --cgroupns=host -v /sys/fs/cgroup:/sys/fs/cgroup:rw -v /var/lib/containerd antmelekhin/docker-systemd:almalinux-10`);
const latestReleasedVersion = (await cli.exec('wget -q https://registry.hub.docker.com/v2/repositories/percona/pmm-client/tags -O - | jq -r .results[].name | grep -v latest | sort -V | tail -n1')).stdout;
await expect(async () => {
const status = await cli.exec(`docker inspect -f '{{.State.Running}}' ${containerName}`);
expect(status.stdout.trim()).toBe('true');
}).toPass({ intervals: [1_000], timeout: 60_000 });
const latestReleasedVersion = (await cli.exec('wget -q https://registry.hub.docker.com/v2/repositories/percona/pmm-client/tags -O - | jq -r .results[].name | grep -v latest | sort -V | tail -n1')).stdout.trim();
await cli.exec(`docker cp ../package_tests/scripts/pmm3_client_install_tarball.sh ${containerName}:/`);
await cli.exec(`docker exec ${containerName} dnf install -y wget`);
await cli.exec(`docker exec ${containerName} /pmm3_client_install_tarball.sh -v ${latestReleasedVersion}`);
await (await cli.exec(`docker exec ${containerName} /pmm3_client_install_tarball.sh -v '${latestReleasedVersion}'`)).assertSuccess();
await cli.exec(`docker exec ${containerName} pmm-agent setup --config-file=/usr/local/percona/pmm/config/pmm-agent.yaml --force --server-insecure-tls --server-address=pmm-server:8443 --server-username=admin --server-password=admin 127.0.0.1 generic tarball_node`);
await cli.exec(`docker exec -d ${containerName} pmm-agent --debug --config-file=/usr/local/percona/pmm/config/pmm-agent.yaml`);
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=`);

@coderabbitai coderabbitai Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.


await adminStatus.outContains('Connected');
await oldVersion.outContains(latestReleasedVersion);
const tarballURL = process.env.PMM_CLIENT_VERSION!.includes('http') ? process.env.PMM_CLIENT_VERSION : 'https://pmm-build-cache.s3.us-east-2.amazonaws.com/PR-BUILDS/pmm-client/pmm-client-latest.tar.gz';

await cli.exec(`docker exec ${containerName} /pmm3_client_install_tarball.sh -v ${tarballURL} -u`);
await cli.exec(`docker exec ${containerName} pkill -f pmm-agent`);
await (await cli.exec(`docker exec ${containerName} /pmm3_client_install_tarball.sh -v '${tarballURL}' -u`)).assertSuccess();
await cli.exec(`docker exec ${containerName} pkill -9 -f pmm-agent || true`);
await expect(async () => {
const ps = await cli.exec(`docker exec ${containerName} pgrep -c pmm-agent || true`);
expect(parseInt(ps.stdout.trim(), 10) || 0).toBe(0);
}).toPass({ intervals: [500], timeout: 10_000 });
await cli.exec(`docker exec -d ${containerName} pmm-agent --debug --config-file=/usr/local/percona/pmm/config/pmm-agent.yaml`);

const newPid = await cli.exec(`docker exec ${containerName} ps -C pmm-agent -o pid=`);
const latestVersion = (await cli.exec('curl -s https://raw.githubusercontent.com/Percona-Lab/pmm-submodules/v3/VERSION')).stdout.trim();
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);
Comment on lines 612 to +623

@coderabbitai coderabbitai Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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 cli.exec(`docker rm -f ${containerName}`);

@coderabbitai coderabbitai Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

});
});
Loading