many: expose component constraints from validation sets #14592

andrewphelpsj · 2024-10-09T16:09:54Z

This refactor enables us to get component constraints out of snapasserts.ValidationSets. I also removed some of the old methods that can be replaced by these new ones.

codecov · 2024-10-09T16:27:56Z

Codecov Report

Attention: Patch coverage is 85.18519% with 16 lines in your changes missing coverage. Please review.

Project coverage is 78.96%. Comparing base (96ea7b0) to head (28fe47e).
Report is 24 commits behind head on master.

Files with missing lines	Patch %	Lines
asserts/snapasserts/validation_sets.go	84.93%	7 Missing and 4 partials ⚠️
image/image_linux.go	50.00%	2 Missing and 1 partial ⚠️
overlord/snapstate/snapstate.go	87.50%	1 Missing ⚠️
overlord/snapstate/target.go	94.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14592      +/-   ##
==========================================
+ Coverage   78.95%   78.96%   +0.01%     
==========================================
  Files        1084     1085       +1     
  Lines      146638   147146     +508     
==========================================
+ Hits       115773   116192     +419     
- Misses      23667    23725      +58     
- Partials     7198     7229      +31

Flag	Coverage Δ
unittests	`78.96% <85.18%> (+0.01%)`	⬆️

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

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

pedronis

I suggest the first commit here should be its own PR, thanks

andrewphelpsj · 2024-10-15T13:06:33Z

Based on #14352 for now, working on getting that merged now. First commit in this PR is 7515505.

miguelpires

lgtm, just one question

asserts/snapasserts/validation_sets.go

miguelpires · 2024-10-15T15:55:13Z

overlord/snapstate/target.go

@@ -413,14 +414,12 @@ func completeStoreAction(action *store.SnapAction, revOpts RevisionOptions, igno

 		// if the caller didn't provide any validation sets, make sure that
 		// the snap is allowed by all of the enforced validation sets
-		invalidSets, err := vsets.CheckPresenceInvalid(naming.Snap(action.InstanceName))
+		pres, err := vsets.Presence(naming.Snap(action.InstanceName))


Will this get the right presence if we use an instance name?

Good catch, thank you. Seems this has been wrong.

This is fixed in 52db459 and 4d28fda.

After talking with @pedronis, we should explicitly ignore validation sets when working with parallel installs. This wasn't really codified before, we were just getting lucky with how the snapasserts API handled instance names.

andrewphelpsj · 2024-10-16T16:48:29Z

This is now based on #14621, the validation sets changes will end up needing this new code.

miguelpires

thank you

miguelpires · 2024-10-18T15:53:05Z

overlord/snapstate/target.go

+		// TODO: figure out the best way to handle parallel installs and
+		// validation sets. right now, we just ignore them.
+		if isParallelInstall {
+			return nil


Maybe it's worth logging something in case we hit this unexpectedly?

andrewphelpsj · 2024-10-21T20:17:28Z

@pedronis this is ready for your review again.

…component's constraints

…ctly

…llel instance

Co-authored-by: Miguel Pires <[email protected]>

pedronis

thank you, did a pass

overlord/snapstate/snapstate.go

pedronis · 2024-11-12T17:35:09Z

asserts/snapasserts/validation_sets.go

+
+// RequiredComponents returns a set of all of the components that are required
+// to be installed when this snap is installed.
+func (s *SnapPresence) RequiredComponents() map[string]presence {


it's strange to have a public method that has a return that uses an unexported type?

asserts/snapasserts/validation_sets.go

pedronis · 2024-11-12T17:37:09Z

asserts/snapasserts/validation_sets.go

-// The method assumes that validation sets are not in conflict.
-func (v *ValidationSets) CheckPresenceInvalid(snapRef naming.SnapRef) ([]ValidationSetKey, error) {
-	cstrs := v.constraintsForSnap(snapRef)
+type presence struct {


wouldn't it be a bit clearer if this was exported?

Yeah, probably. I was having a hard time naming it, since Presence technically already has a meaning.

asserts/snapasserts/validation_sets.go

…on that vsets must not be in conflict

pedronis

couple of naming suggestions

pedronis · 2024-11-21T19:02:30Z

asserts/snapasserts/validation_sets.go

-	cstrs := v.constraintsForSnap(snapRef)
+// ContainerPresence represents the allowed presence of a snap or component with
+// respect to a set of validation sets that it was derived from.
+type ContainerPresence struct {


we have a PresenceConstraintError alread,y maybe this should be called PresenceContraint ?

pedronis · 2024-11-21T19:03:02Z

asserts/snapasserts/validation_sets.go

+
+// SnapPresence contains information about a snap's allowed presence with
+// respect to a set of validation sets.
+type SnapPresence struct {


SnapPresenceConstraint(s) ?

Done, thanks.

…e-component-constraints

pedronis

thank you

pedronis · 2024-11-25T19:01:17Z

asserts/snapasserts/export_test.go

-func NewSnapPresence(p ContainerPresence, comps map[string]ContainerPresence) SnapPresence {
-	return SnapPresence{
-		ContainerPresence: p,
+func NewSnapPresence(p PresenceContraint, comps map[string]PresenceContraint) SnapPresenceConstraints {


should this also be renamed for consistency?

andrewphelpsj · 2024-11-26T15:41:57Z

The arch failures are a known issue due to arch releasing the 6.12 over the weekend. A fix is being worked on.

andrewphelpsj requested review from pedronis and miguelpires October 9, 2024 16:09

andrewphelpsj force-pushed the expose-component-constraints branch 2 times, most recently from 6bbfc3b to 3c91b0b Compare October 10, 2024 17:02

pedronis requested changes Oct 11, 2024

View reviewed changes

andrewphelpsj force-pushed the expose-component-constraints branch 2 times, most recently from 407b591 to ddb3518 Compare October 15, 2024 13:06

andrewphelpsj force-pushed the expose-component-constraints branch from ddb3518 to 16e7660 Compare October 15, 2024 15:43

miguelpires reviewed Oct 15, 2024

View reviewed changes

andrewphelpsj force-pushed the expose-component-constraints branch from 16e7660 to 92ef00b Compare October 16, 2024 16:29

andrewphelpsj requested a review from miguelpires October 16, 2024 16:47

andrewphelpsj force-pushed the expose-component-constraints branch from 98183e9 to dc522a9 Compare October 16, 2024 19:28

miguelpires approved these changes Oct 18, 2024

View reviewed changes

andrewphelpsj force-pushed the expose-component-constraints branch from dc522a9 to 2ab0208 Compare October 21, 2024 20:17

andrewphelpsj requested a review from pedronis October 21, 2024 20:17

andrewphelpsj force-pushed the expose-component-constraints branch 2 times, most recently from f922b67 to 7664aa9 Compare October 28, 2024 10:10

andrewphelpsj and others added 5 commits November 1, 2024 10:11

a/snapasserts: add new method to ValidationSets to enable checking a …

ebd6e24

…component's constraints

many: remove snapasserts.ValidationSets.CheckPresence* methods

0807a96

a/snapasserts: add check that validation sets API is being used corre…

d333820

…ctly

o/snapstate: explicitly ignore validation sets when installing a para…

2d1c61e

…llel instance

a/snapasserts: fix doc comment

28fe47e

Co-authored-by: Miguel Pires <[email protected]>

andrewphelpsj force-pushed the expose-component-constraints branch from 7664aa9 to 28fe47e Compare November 1, 2024 09:12

pedronis reviewed Nov 12, 2024

View reviewed changes

andrewphelpsj added 2 commits November 18, 2024 15:02

a/snapasserts: rename and export presence to ContainerPresence

210e581

a/snapasserts: update doc comment on ValidationSets.Presence to menti…

8e890e3

…on that vsets must not be in conflict

andrewphelpsj added 2 commits November 18, 2024 15:03

a/snapasserts: reuse bound variable rather than re-calling function

7fb0476

o/snapstate: directly compare snap.Revision structs rather than fields

32e4970

andrewphelpsj requested a review from pedronis November 18, 2024 20:10

pedronis reviewed Nov 21, 2024

View reviewed changes

a/snapasserts: rename constraint types

904bc4b

andrewphelpsj requested a review from pedronis November 25, 2024 17:43

Merge branch 'master' of https://github.com/snapcore/snapd into expos…

e1b9399

…e-component-constraints

pedronis approved these changes Nov 25, 2024

View reviewed changes

fixup! a/snapasserts: rename constraint types

4b66b33

alfonsosanchezbeato merged commit beac918 into canonical:master Nov 26, 2024
50 of 54 checks passed

andrewphelpsj deleted the expose-component-constraints branch November 26, 2024 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

many: expose component constraints from validation sets #14592

many: expose component constraints from validation sets #14592

andrewphelpsj commented Oct 9, 2024

codecov bot commented Oct 9, 2024 •

edited

Loading

pedronis left a comment

andrewphelpsj commented Oct 15, 2024 •

edited

Loading

miguelpires left a comment

miguelpires Oct 15, 2024

andrewphelpsj Oct 16, 2024 •

edited

Loading

andrewphelpsj Oct 16, 2024 •

edited

Loading

andrewphelpsj commented Oct 16, 2024

miguelpires left a comment

miguelpires Oct 18, 2024

andrewphelpsj commented Oct 21, 2024

pedronis left a comment

pedronis Nov 12, 2024

pedronis Nov 12, 2024

andrewphelpsj Nov 18, 2024

pedronis left a comment

pedronis Nov 21, 2024

pedronis Nov 21, 2024

andrewphelpsj Nov 25, 2024

pedronis left a comment

pedronis Nov 25, 2024

andrewphelpsj commented Nov 26, 2024

many: expose component constraints from validation sets #14592

many: expose component constraints from validation sets #14592

Conversation

andrewphelpsj commented Oct 9, 2024

codecov bot commented Oct 9, 2024 • edited Loading

Codecov Report

pedronis left a comment

Choose a reason for hiding this comment

andrewphelpsj commented Oct 15, 2024 • edited Loading

miguelpires left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewphelpsj Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

andrewphelpsj Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

andrewphelpsj commented Oct 16, 2024

miguelpires left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewphelpsj commented Oct 21, 2024

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewphelpsj commented Nov 26, 2024

codecov bot commented Oct 9, 2024 •

edited

Loading

andrewphelpsj commented Oct 15, 2024 •

edited

Loading

andrewphelpsj Oct 16, 2024 •

edited

Loading

andrewphelpsj Oct 16, 2024 •

edited

Loading