Skip to content

PMM-15295 Warn on Nomad without public address. - #5849

Open
JiriCtvrtka wants to merge 3 commits into
mainfrom
PMM-15295-warn-nomad-without-public-address
Open

PMM-15295 Warn on Nomad without public address.#5849
JiriCtvrtka wants to merge 3 commits into
mainfrom
PMM-15295-warn-nomad-without-public-address

Conversation

@JiriCtvrtka

@JiriCtvrtka JiriCtvrtka commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Ticket number: PMM-15295

Feature build: SUBMODULES-0

What

Emit a configuration warning when PMM_ENABLE_NOMAD is set but PMM_PUBLIC_ADDRESS is not, instead of silently skipping the Nomad server.

Why

Nomad needs both variables. With the flag alone, Settings.IsNomadEnabled() returns false and supervisord skips the nomad-server template — no error, no warning. The container is healthy and PMM works, so nothing indicates why nothing can be executed.

This cost real debugging time during SEP integration testing: SEP dispatches task execution through PMM's embedded Nomad, so the failure surfaces as "SEP cannot run anything", several layers away from one unset variable. The requirement is documented in documentation/docs/reference/nomad.md, but the runtime said nothing.

How

A post-loop cross-check in ParseEnvVars appending to the existing warns list — no new plumbing:

if envSettings.EnableNomad != nil && *envSettings.EnableNomad &&
    (envSettings.PMMPublicAddress == nil || *envSettings.PMMPublicAddress == "") {
    warns = append(warns, "PMM_ENABLE_NOMAD is set but PMM_PUBLIC_ADDRESS is not; Nomad will not start")
}

The warning reaches the operator through the same channel as every other environment-variable warning: pmm-managed-init/main.go logs it as Configuration warning: %s, and server.go via s.l.Warnln on UpdateSettingsFromEnv.

This is a diagnostic, not a new failure mode. Only warns is appended to; errs is untouched, so pmm-managed-init does not exit and PMM starts exactly as before.

Behaviour

Environment | Warning -- | -- PMM_ENABLE_NOMAD=1, no public address | yes PMM_ENABLE_NOMAD=1, PMM_PUBLIC_ADDRESS= (empty) | yes — IsNomadEnabled() also treats empty as disabled PMM_ENABLE_NOMAD=1 + public address | no PMM_ENABLE_NOMAD=0 | no Neither set | no Public address only | no PMM_ENABLE_NOMAD=maybe | no — the existing parse error already reports it

Testing

TestNomadWithoutPublicAddress in managed/utils/envvars/parser_test.go covers every row above. Statement and branch coverage of the new block is complete, and each sub-term of the predicate is exercised both true and false.

  • go test ./managed/utils/envvars/... — pass
  • bin/golangci-lint run managed/utils/envvars/... — 0 issues
  • go build ./managed/..., go vet — clean

Out of scope

Changing the requirement itself — Nomad genuinely needs the public address.

@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 45.56%. Comparing base (31318c7) to head (1b8f7b4).
⚠️ Report is 158 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5849      +/-   ##
==========================================
+ Coverage   43.59%   45.56%   +1.97%     
==========================================
  Files         415      217     -198     
  Lines       43134    28116   -15018     
==========================================
- Hits        18804    12811    -5993     
+ Misses      22454    13920    -8534     
+ Partials     1876     1385     -491     

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

@JiriCtvrtka
JiriCtvrtka marked this pull request as ready for review August 31, 2026 11:03
@JiriCtvrtka
JiriCtvrtka requested a review from a team as a code owner August 31, 2026 11:03
@JiriCtvrtka
JiriCtvrtka requested review from 4nte and ademidoff and removed request for a team August 31, 2026 11:03
@coderabbitai

coderabbitai Bot commented Aug 31, 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: Pro Plus

Run ID: 9b5bfc7d-204e-4c94-b261-eb3547addb5e

📥 Commits

Reviewing files that changed from the base of the PR and between 95c70a4 and 1b8f7b4.

📒 Files selected for processing (2)
  • managed/utils/envvars/parser.go
  • managed/utils/envvars/parser_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

ParseEnvVars now appends a warning when PMM_ENABLE_NOMAD is enabled and PMM_PUBLIC_ADDRESS is unset or empty. The warning states that Nomad will not start without a configured public address. TestNomadWithoutPublicAddress verifies warning presence and absence across seven environment-variable combinations. It also verifies that an invalid Nomad value returns one error and no warning.

Merge Risk: ⚪ Minimal · up to 1b8f7

This change warns operators when Nomad is enabled without the required public address, without altering service behavior or access controls. No actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: warning when Nomad is enabled without a public address.
Description check ✅ Passed The description includes the required ticket number and feature build, explains what, why, and how, documents behavior and testing, and identifies out-of-scope work. The API documentation checkbox is …
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 required ticket number and feature build, explains what, why, and how, documents behavior and testing, and identifies out-of-scope work. The API documentation checkbox is not required because this PR does not alter API endpoints.


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.

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.

1 participant