Skip to content

PMM-15304 Fix node type detection - #5870

Open
ademidoff wants to merge 4 commits into
mainfrom
PMM-15304-fix-node-type-detection
Open

PMM-15304 Fix node type detection#5870
ademidoff wants to merge 4 commits into
mainfrom
PMM-15304-fix-node-type-detection

Conversation

@ademidoff

@ademidoff ademidoff commented Sep 1, 2026

Copy link
Copy Markdown
Member

Ticket number: PMM-15304

Feature build: Percona-Lab/pmm-submodules#4555

PMM Server always runs in a container, yet its own Inventory Node showed up as generic — most visibly in an HA deployment, where every pmm-ha-* replica sat next to container client nodes with the wrong type.

Two independent causes:

  1. The PMM Server Node was never detected at all. managed/models/database.go:1640 (HA) and managed/models/database.go:1672 (single server) hardcoded GenericNodeType in the setup fixtures. The internal pmm-agent is set up with --skip-registration, so it never goes through the client path where a node type is chosen — pmm-managed writes the row itself.
  2. checkContainer() in utils/nodeinfo/nodeinfo.go was broken on cgroup v2. It only looked for /docker/ or /lxc/ in /proc/1/cgroup, which under cgroup v2 holds just 0::/. Modern Docker, Podman and Kubernetes all read as a plain host. It now also checks the runtime marker files (/.dockerenv, /run/.containerenv), the container and KUBERNETES_SERVICE_HOST environment variables, and a wider set of cgroup v1 paths (/kubepods, containerd, crio-, libpod).

Also fixed along the way: pmm-admin register hardcoded default:"generic" for its node-type argument instead of the ${nodeTypeDefault} Kong variable that admin/cmd/bootstrap.go already computes and pmm-admin config already honors.

Existing Nodes are deliberately left alone — no migration. Only fresh installs get the corrected fixture.

Verification

  • Cross-compiled pmm-admin and ran it inside a real cgroup v2 Docker container: [<node-type>] Node type. One of: [generic,container]. Default: container. On a macOS host it still reports generic.
  • New table test TestCheckContainer in utils/nodeinfo/nodeinfo_test.go covers host/Docker/Podman/LXC/Kubernetes under both cgroup versions against a fake filesystem root. It replaces the require.False(t, info.Container) assertion in TestGet, which only held because detection was broken and would now fail whenever the suite runs inside a container (make env TARGET=test-common).
  • Updated the tests that asserted the fixture Node's type: managed/models/node_helpers_test.go (including FindNodesByType, where PMM Server now matches the container filter), managed/services/management/node_test.go and managed/services/qan/client_test.go.
  • managed/models green via DB-backed run; managed/services/{qan,inventory,management,agents} and managed/services green against a live PostgreSQL.

PMM Server always runs in a container, but its own Inventory Node was
created as "generic": the setup fixtures hardcoded GenericNodeType, both
for a single server and for every HA replica.

The client-side heuristic behind the node-type default was broken too.
checkContainer() only looked for "/docker/" or "/lxc/" in /proc/1/cgroup,
which under cgroup v2 holds just "0::/" - so Docker, Podman and
Kubernetes all read as a plain host. It now checks the runtime marker
files, the "container" and KUBERNETES_SERVICE_HOST variables, and a wider
set of cgroup v1 paths.

pmm-admin register hardcoded "generic" as its default instead of using
the detected ${nodeTypeDefault} that pmm-admin config already honors.

Existing Nodes keep the type they were registered with; only fresh
installs get the corrected fixture.

Signed-off-by: Alex Demidoff <a@demidoff.me>
@ademidoff
ademidoff requested a review from a team as a code owner September 1, 2026 10:54
@ademidoff
ademidoff requested review from 4nte and JiriCtvrtka and removed request for a team September 1, 2026 10:54
@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 45.90%. Comparing base (31318c7) to head (83f742c).
⚠️ Report is 161 commits behind head on main.

Files with missing lines Patch % Lines
managed/models/database.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5870      +/-   ##
==========================================
+ Coverage   43.59%   45.90%   +2.30%     
==========================================
  Files         415      417       +2     
  Lines       43134    43689     +555     
==========================================
+ Hits        18804    20054    +1250     
+ Misses      22454    21646     -808     
- Partials     1876     1989     +113     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 4955a281-9517-4dd1-892a-67ea1361ead9

📥 Commits

Reviewing files that changed from the base of the PR and between b558d20 and 54f3ea6.

📒 Files selected for processing (2)
  • utils/nodeinfo/nodeinfo.go
  • utils/nodeinfo/nodeinfo_test.go
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • percona/pmm-qa (manual)
  • percona/pmm (manual)

Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.


Walkthrough

The change expands container detection for Docker, Podman, LXC, Kubernetes, environment markers, and cgroup identifiers. nodeinfo.Get now uses a testable filesystem root. PMM Server nodes created during HA and standard setup use ContainerNodeType. Registration resolves the default node type from ${nodeTypeDefault}. Related tests now expect the container node type.

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant nodeinfo.Get
  participant checkContainer
  participant Filesystem
  participant Environment
  Caller->>nodeinfo.Get: Request node information
  nodeinfo.Get->>checkContainer: Pass root filesystem
  checkContainer->>Filesystem: Read runtime markers and cgroup data
  checkContainer->>Environment: Inspect container variables
  checkContainer-->>nodeinfo.Get: Return container status
  nodeinfo.Get-->>Caller: Return node information
Loading

Merge Risk: ⚪ Minimal · up to 54f3e

This PR corrects container node classification and registration defaults without introducing an actionable merge-blocking risk; it is merge-ready after normal checks and review.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main change: fixing PMM node type detection. It includes the relevant ticket number.
Description check ✅ Passed The description includes the ticket number, feature build link, problem statement, root causes, implementation details, scope limitations, and verification results. The optional related-work section i…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description includes the ticket number, feature build link, problem statement, root causes, implementation details, scope limitations, and verification results. The optional related-work section is not required, and no API endpoint changes are described.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

ademidoff and others added 2 commits September 1, 2026 14:41
systemd sets "container" in PID 1's environment only and strips it from
the services it starts, so an agent running as a unit inside LXC or LXD
on cgroup v2 saw no signal at all. Check /run/systemd/container, which
systemd writes when it boots as PID 1 in a container.

Also match /system.slice/docker-<id>.scope, the path form the systemd
cgroup driver produces, which "/docker/" cannot.

Signed-off-by: Alex Demidoff <a@demidoff.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants