fix: take Pebble defaults into consideration when consistency checking Checks by tonyandrewmeyer · Pull Request #2567 · canonical/operator

tonyandrewmeyer · 2026-06-12T09:05:09Z

When Pebble returns information about a check, it copies the level, startup, and threshold into the response from the computed plan. If none of the layers have provided values for these, the plan has defaults (unset, enabled, and 3, respectively).

When we check a CheckInfo for consistency in Scenario (meaning that the values should be ones Pebble could sensibly return, given the rest of the state), we need to take that default value into account, as Pebble would. We don't actually model checks going up and down currently, so it's really only the consistency that we are concerned with.

This PR fixes the consistency check so that it will fall back to the Pebble defaults when needed. It repeats the defaults, because the existing copy is buried in the mock Pebble client in Harness, and the source of truth is in the Pebble code.

Fixes #2566

… the data Pebble copies from the plan is using defsults in the layer.

…ensure we use the default values when necessary.

Co-authored-by: Dave Wilding <tech@dpw.me>

james-garner-canonical · 2026-06-15T01:15:11Z

+            # an absent value on the check info as also matching the default.
+            unset_defaults = {
+                'level': ({None, pebble.CheckLevel.UNSET}, pebble.CheckLevel.UNSET),
+                'startup': ({None, pebble.CheckStartup.UNSET}, pebble.CheckStartup.ENABLED),


Pebble seems to technically default to UNSET -- in Check.__init__: self.startup = CheckStartup(dct.get('startup', '')). Though apparently they're functionally equivalent ...

class CheckStartup(enum.Enum): """Enum of check startup options.""" UNSET = '' # Note that this is treated as ENABLED. ENABLED = 'enabled' DISABLED = 'disabled'

Interestingly for this change though, if we use UNSET as the default, then our _normalise logic becomes is not None, and the need for the helper and the set of 'unset' values seems to go away.

For reference:

Pebble:

Combined plan defaults: Threshold → 3 (in plan.go). Level and Startup are not defaulted in the combined plan, they stay as UnsetLevel/CheckStartupUnknown ("", in the Go way of doing things).

/v1/checks CheckInfo response (checkstate/manager.go): Startup is normalised server-side: Unknown → Enabled. Level passes through unchanged. Threshold passes through (so always 3 from the combined plan).

(I wish this worked differently, so startup ended up as enabled in the combined plan, even though it would be unset in the layers so combining works. Then the plan would reflect better what happens, and it'd be more consistent with threshold. But it can't be changed now.)

ops.pebble:

Check: startup = CheckStartup(dct.get('startup', '')) → UNSET. threshold = dct.get('threshold') → None when not in layer. level → UNSET.

CheckInfo.from_dict: startup = CheckStartup(d.get('startup', 'enabled')). Defaults to ENABLED if the key is missing (so it can never be UNSET from a real Pebble response, since Pebble sends enabled/disabled; and even if the key were missing, ops defaults to ENABLED). level → UNSET if missing. threshold is a required key.

scenario:

CheckInfo has different defaults than real Pebble:

level default: None (not CheckLevel.UNSET)

startup default: CheckStartup.ENABLED (not UNSET)

threshold default: 3 (int, never None)

(I think I did a poor job when I originally added this. It would probably make more sense to mirror Pebble more closely. But also not really changeable now.)

Harness:

passes check.level/check.startup/check.threshold (None→3) straight through to pebble.CheckInfo.

I'm good unrolling the loop as per another thread. Putting the logic there probably makes it clear enough?

james-garner-canonical · 2026-06-15T01:19:56Z

+
+            for attr, (unset, default) in unset_defaults.items():
+                check_value = _normalise(getattr(check, attr), unset, default)
+                plan_value = _normalise(getattr(plan_check, attr), unset, default)


Do we need to explicitly normalise the plan value, or is this actually a no-op (or even counterproductive) since State.containers.plan ultimately is populated by calling the real pebble.Plan?

I'm a little unsure here because of the private-looking Container._base_plan attribute -- if users never pass this, then our defaults all come from the real ops.pebble.Plan directly. If they do pass it, then that's funky, but maybe whatever they pass in should be the ground truth for comparison rather than attempting to normalise it.

The thing is that Container.plan does build a real pebble.Plan from _base_plan, but _render_checks then writes the layer's pebble.Check objects straight into plan.checks[name], bypassing Pebble's combine/defaulting logic for checks. So plan.checks[name].threshold stays None, and .startup/.level stay UNSET, whenever the layer didn't set them. That's what motivated the threshold default. So the plan-side normalisation is needed.

Possibly the way Container.plan works could be changed, the various render methods are mostly taken from pebble at the moment, but I'm not sure I want to do that big a change.

…arisons. This drops the _normalise helper and the unset/defaults mapping in favour of three direct comparisons, one per attribute, so type checking covers each branch and the per-attribute normalisation rules are obvious at the call site. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

The previous commit only normalised the plan side, but scenario users can pass CheckInfo.startup=UNSET (or None) and CheckInfo.threshold=None too, so the check side needs the same treatment to avoid spurious mismatches when both sides are effectively unset. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Like test_stop_checks, this test calls Container.stop_checks, which logs a security event via _log_security_event and emits a RuntimeWarning when logging is not configured for Harness in these tests. Wrap the stop_checks call in pytest.warns(RuntimeWarning) so the warning does not surface as a test failure under -W error. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

…deterministic. ops.log._get_juju_log_and_app_id is functools.cached, so the RuntimeWarning emitted when no JujuLogHandler is set up only fires the first time it is called in a Python process. Under pytest-xdist, two harness tests in the same worker that both used pytest.warns(RuntimeWarning) around Container.stop_checks could race: whichever test ran first got the warning, and the second failed with "DID NOT WARN". Mirror the reset_security_logging fixture from testing/tests/test_e2e/test_pebble.py, applied to both test_stop_checks and test_stop_then_start, so the cache is cleared before and after each test and the warning fires deterministically. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

james-garner-canonical · 2026-06-18T00:28:49Z

+        """
+        _get_juju_log_and_app_id.cache_clear()
+        yield
+        _get_juju_log_and_app_id.cache_clear()


Clearing again afterwards seems like it might just hide other tests forgetting to request this fixture when they really should.

james-garner-canonical

Looks good, thanks!

james-garner-canonical · 2026-06-18T00:31:44Z

+            # The scenario CheckInfo uses None for an absent level, while the
+            # plan uses CheckLevel.UNSET, so those two also need to compare
+            # equal.
+            check_level = check.level if check.level is not None else pebble.CheckLevel.UNSET


Thanks, I like the unrolled form. WDYT about localising comments per attribute?

Suggested change

# The scenario CheckInfo uses None for an absent level, while the

# plan uses CheckLevel.UNSET, so those two also need to compare

# equal.

check_level = check.level if check.level is not None else pebble.CheckLevel.UNSET

# Scenario allows None for unset, Pebble defaults to UNSET.

check_level = check.level if check.level is not None else pebble.CheckLevel.UNSET

james-garner-canonical · 2026-06-18T00:32:33Z

+                    f'container {container.name!r} has a check {check.name!r} with a '
+                    f"different 'level' ({check.level}) than the plan ({plan_check.level}).",
+                )
+            check_startup = (


Suggested change

check_startup = (

# Scenario defaults to None, Pebble defaults to UNSET and collapses to ENABLED.

check_startup = (

james-garner-canonical · 2026-06-18T00:33:10Z

+                    f'container {container.name!r} has a check {check.name!r} with a '
+                    f"different 'startup' ({check.startup}) than the plan ({plan_check.startup}).",
+                )
+            check_threshold = 3 if check.threshold is None else check.threshold


Suggested change

check_threshold = 3 if check.threshold is None else check.threshold

# Scenario and Pebble allow None, collapsing to 3.

check_threshold = 3 if check.threshold is None else check.threshold

tonyandrewmeyer added 3 commits June 12, 2026 20:56

Add a test that CheckInfos are correctly checked for consistency when…

efbd1ee

… the data Pebble copies from the plan is using defsults in the layer.

For the fields Pebble copies into check info from the computed plan, …

ee7c7b6

…ensure we use the default values when necessary.

Merge branch 'main' into fix-2566

bd1ed6e

tonyandrewmeyer marked this pull request as ready for review June 12, 2026 09:10

tonyandrewmeyer requested review from dwilding and james-garner-canonical June 12, 2026 09:11

dwilding approved these changes Jun 12, 2026

View reviewed changes

Comment thread testing/tests/test_consistency_checker.py Outdated

Apply suggestion from @dwilding

6a5cf3e

Co-authored-by: Dave Wilding <tech@dpw.me>

james-garner-canonical reviewed Jun 15, 2026

View reviewed changes

tonyandrewmeyer and others added 2 commits June 17, 2026 09:31

Merge branch 'main' into fix-2566

0db680d

tonyandrewmeyer requested a review from james-garner-canonical June 16, 2026 21:33

tonyandrewmeyer and others added 4 commits June 17, 2026 09:34

style: wrap overlong line in threshold consistency error message

c781050

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

james-garner-canonical reviewed Jun 18, 2026

View reviewed changes

james-garner-canonical approved these changes Jun 18, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: take Pebble defaults into consideration when consistency checking Checks#2567

fix: take Pebble defaults into consideration when consistency checking Checks#2567
tonyandrewmeyer wants to merge 10 commits into
canonical:mainfrom
tonyandrewmeyer:fix-2566

tonyandrewmeyer commented Jun 12, 2026 •

edited

Loading

Uh oh!

Uh oh!

james-garner-canonical Jun 15, 2026

Uh oh!

tonyandrewmeyer Jun 16, 2026

Uh oh!

tonyandrewmeyer Jun 16, 2026

Uh oh!

james-garner-canonical Jun 15, 2026

Uh oh!

tonyandrewmeyer Jun 16, 2026

Uh oh!

Uh oh!

james-garner-canonical Jun 18, 2026

Uh oh!

james-garner-canonical left a comment

Uh oh!

james-garner-canonical Jun 18, 2026

Uh oh!

james-garner-canonical Jun 18, 2026

Uh oh!

james-garner-canonical Jun 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	check_startup = (
	# Scenario defaults to None, Pebble defaults to UNSET and collapses to ENABLED.
	check_startup = (

	check_threshold = 3 if check.threshold is None else check.threshold
	# Scenario and Pebble allow None, collapsing to 3.
	check_threshold = 3 if check.threshold is None else check.threshold

Conversation

tonyandrewmeyer commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

james-garner-canonical left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tonyandrewmeyer commented Jun 12, 2026 •

edited

Loading