Skip to content

o/assertstate: Commit fetches asserts and checks enforcement - #17377

Open
miguelpires wants to merge 3 commits into
canonical:masterfrom
miguelpires:confdb-fail-enforce
Open

o/assertstate: Commit fetches asserts and checks enforcement#17377
miguelpires wants to merge 3 commits into
canonical:masterfrom
miguelpires:confdb-fail-enforce

Conversation

@miguelpires

@miguelpires miguelpires commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Use (TryEnforced|Monitor)ValidationSets to fetch validation sets and perform checks (enforcement or against model) before changing state. Since changes made through confdb can affect many validation sets, it's important to note that changes are not atomic. We'll first check if all the enforce changes can be applied and abort, if not. This is the most frequent failure scenario so it's the first one to be checked. From then on, if a monitor or forget change fails, the previous changes are still persisted. This could be due to model checks failing or a failure to fetch a validation set.
We could make these changes atomic, but it would require quite a bit of special-purpose code, so I didn't think it was was worth it, since the original API doesn't support it anyway.

https://warthogs.atlassian.net/browse/SNAPDENG-37233

@miguelpires miguelpires added the confdb confdb work (previously called registries and before aspects) label Jul 16, 2026
@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 70 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.85%. Comparing base (8bbab2e) to head (93df1a9).
⚠️ Report is 59 commits behind head on master.

Files with missing lines Patch % Lines
overlord/assertstate/assertstatetest/fakestore.go 57.74% 50 Missing and 10 partials ⚠️
overlord/assertstate/confdb/validation_sets.go 85.29% 8 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #17377      +/-   ##
==========================================
- Coverage   78.85%   78.85%   -0.01%     
==========================================
  Files        1404     1409       +5     
  Lines      196234   197296    +1062     
  Branches     2466     2561      +95     
==========================================
+ Hits       154748   155585     +837     
- Misses      32191    32365     +174     
- Partials     9295     9346      +51     
Flag Coverage Δ
unittests 78.85% <66.66%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown

Thu Jul 23 18:47:25 UTC 2026

Spread tests skipped

@miguelpires miguelpires changed the title o/a/confdb: Commit fetches asserts and checks enforcement o/assertstate: Commit fetches asserts and checks enforcement Jul 17, 2026
@miguelpires
miguelpires force-pushed the confdb-fail-enforce branch from 028f78f to ebfb6d6 Compare July 17, 2026 09:35

@pedronis pedronis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

naming question

type valsetChange struct {
// kind can be "monitor", "enforce" or "forget"
kind string
id valsetRef

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

both the name and the type of this are a bit confusing. A close thing we have is asserts.AtSequence, maybe there's a name more like that we can use?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the valsetChange, the valsetRef or both?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sorry I meant the name "id" and the type name "valsetRef"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I renamed it to atSequence and documented the fields better. Sorry, I had to force push to remove another commit I added to this branch by mistake

Signed-off-by: Miguel Pires <miguel.pires@canonical.com>
Use (TryEnforced|Monitor)ValidationSets to fetch validation sets and
perform checks (enforcement or against model) before changing state.
Since changes made through confdb can affect many validation sets,
it's important to note that changes are not atomic. We'll first check
if all the enforce changes can be applied and abort, if not. These
is the most frequent failure scenario so it's the first one to be
checked. From then on, if a monitor or forget change fails, the
previous changes are still persisted. This could be due to model checks
failing or a failure to fetch a validation set.
We can make these changes atomic, but it would require quite a bit of
special-purpose code, so I didn't think it was was worth it, since the
original API doesn't support it anyway.

Signed-off-by: Miguel Pires <miguel.pires@canonical.com>
Signed-off-by: Miguel Pires <miguel.pires@canonical.com>
@miguelpires
miguelpires force-pushed the confdb-fail-enforce branch from 6575e0a to 93df1a9 Compare July 22, 2026 09:58
@miguelpires
miguelpires requested a review from pedronis July 22, 2026 10:00

@pedronis pedronis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thx, small further naming remark

// kind denotes the type of change which can be "monitor", "enforce" or "forget".
kind string
// valsetID identifies the validation-set, possibly pinning to a sequence.
valsetID atSequence

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

valsetPt perhaps, as in Pt meaning point aka sequence point

@andrewphelpsj andrewphelpsj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@github-actions github-actions Bot added the Auto rerun spread Auto reruns spread up to 4 times in non-draft PRs w/ >=1 approval and <20 fails in any fund. system label Jul 23, 2026
@miguelpires miguelpires added the Skip spread Indicate that spread job should not run label Jul 23, 2026
@miguelpires miguelpires reopened this Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Auto rerun spread Auto reruns spread up to 4 times in non-draft PRs w/ >=1 approval and <20 fails in any fund. system confdb confdb work (previously called registries and before aspects) Skip spread Indicate that spread job should not run

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants