Make sure to stop the time span before returning job's outcome.
This was identified as a data race where the DeviceManager.ensureSeeded() would
call seed20.LoadMeta(), which internally loads seed meta and launches load-meta
jobs. When exuction goes back to in ensureSeeded(), the timings would be saved
to state, which causes flattening of all time spans. Since load-meta jobs were
running, and the code in seed20.doLoadMeta() only synchronized with results from
outcomeCh, there was no guarantee that TimeSpan.Stop() ran before we exited the
function.
Backtrace from go test -race:
```
WARNING: DATA RACE
Read at 0x00c0000f06c8 by goroutine 14:
runtime.racereadrange()
<autogenerated>:1 +0x1b
github.com/snapcore/snapd/timings.flattenRecursive()
/home/maciek/work/canonical/snapd/timings/state.go:113 +0x53c
github.com/snapcore/snapd/timings.flattenRecursive()
/home/maciek/work/canonical/snapd/timings/state.go:113 +0x53c
github.com/snapcore/snapd/timings.(*Timings).flatten()
/home/maciek/work/canonical/snapd/timings/state.go:87 +0x22c
github.com/snapcore/snapd/timings.(*Timings).Save()
/home/maciek/work/canonical/snapd/timings/state.go:140 +0x152
github.com/snapcore/snapd/overlord/devicestate.(*DeviceManager).ensureSeeded()
/home/maciek/work/canonical/snapd/overlord/devicestate/devicemgr.go:1007 +0x6d7
github.com/snapcore/snapd/overlord/devicestate.(*DeviceManager).Ensure()
/home/maciek/work/canonical/snapd/overlord/devicestate/devicemgr.go:1727 +0x4a
github.com/snapcore/snapd/overlord.(*StateEngine).Ensure()
/home/maciek/work/canonical/snapd/overlord/stateengine.go:147 +0x333
github.com/snapcore/snapd/overlord.(*Overlord).settle()
/home/maciek/work/canonical/snapd/overlord/overlord.go:553 +0x285
github.com/snapcore/snapd/overlord.(*Overlord).Settle()
/home/maciek/work/canonical/snapd/overlord/overlord.go:596 +0x15f1
github.com/snapcore/snapd/overlord/devicestate_test.(*firstBoot20Suite).testPopulateFromSeedCore20ValidationSetTracking()
/home/maciek/work/canonical/snapd/overlord/devicestate/firstboot20_test.go:1259 +0x15af
github.com/snapcore/snapd/overlord/devicestate_test.(*firstBoot20Suite).TestPopulateFromSeedCore20ValidationSetTrackingFailsUnmetCriterias()
/home/maciek/work/canonical/snapd/overlord/devicestate/firstboot20_test.go:1386 +0xa11
runtime.call16()
/snap/go/10029/src/runtime/asm_amd64.s:701 +0x48
reflect.Value.Call()
/snap/go/10029/src/reflect/value.go:339 +0xd7
gopkg.in/check%2ev1.(*suiteRunner).forkTest.func1()
/home/maciek/work/canonical/snapd/vendor/gopkg.in/check.v1/check.go:775 +0xa71
gopkg.in/check%2ev1.(*suiteRunner).forkCall.func1()
/home/maciek/work/canonical/snapd/vendor/gopkg.in/check.v1/check.go:669 +0xe5
Previous write at 0x00c0000f06c8 by goroutine 74:
github.com/snapcore/snapd/timings.(*Span).Stop()
/home/maciek/work/canonical/snapd/timings/timings.go:119 +0xee
github.com/snapcore/snapd/seed.(*seed20).doLoadMeta.func5.1()
/home/maciek/work/canonical/snapd/seed/seed20.go:586 +0x39
runtime.deferreturn()
/snap/go/10029/src/runtime/panic.go:436 +0x32
Goroutine 14 (running) created at:
gopkg.in/check%2ev1.(*suiteRunner).forkCall()
/home/maciek/work/canonical/snapd/vendor/gopkg.in/check.v1/check.go:666 +0x584
gopkg.in/check%2ev1.(*suiteRunner).forkTest()
/home/maciek/work/canonical/snapd/vendor/gopkg.in/check.v1/check.go:757 +0x164
gopkg.in/check%2ev1.(*suiteRunner).runTest()
/home/maciek/work/canonical/snapd/vendor/gopkg.in/check.v1/check.go:812 +0x414
gopkg.in/check%2ev1.(*suiteRunner).run()
/home/maciek/work/canonical/snapd/vendor/gopkg.in/check.v1/check.go:618 +0x3bf
gopkg.in/check%2ev1.Run()
/home/maciek/work/canonical/snapd/vendor/gopkg.in/check.v1/run.go:92 +0x49
gopkg.in/check%2ev1.RunAll()
/home/maciek/work/canonical/snapd/vendor/gopkg.in/check.v1/run.go:84 +0x127
gopkg.in/check%2ev1.TestingT()
/home/maciek/work/canonical/snapd/vendor/gopkg.in/check.v1/run.go:72 +0x571
github.com/snapcore/snapd/overlord/devicestate_test.TestDeviceManager()
/home/maciek/work/canonical/snapd/overlord/devicestate/devicestate_test.go:80 +0x2e
testing.tRunner()
/snap/go/10029/src/testing/testing.go:1439 +0x213
testing.(*T).Run.func1()
/snap/go/10029/src/testing/testing.go:1486 +0x47
Goroutine 74 (finished) created at:
github.com/snapcore/snapd/seed.(*seed20).doLoadMeta()
/home/maciek/work/canonical/snapd/seed/seed20.go:585 +0x664
github.com/snapcore/snapd/seed.(*seed20).LoadMeta()
/home/maciek/work/canonical/snapd/seed/seed20.go:703 +0x290
github.com/snapcore/snapd/overlord/devicestate.(*DeviceManager).populateStateFromSeedImpl.func2()
/home/maciek/work/canonical/snapd/overlord/devicestate/firstboot.go:170 +0x8c
github.com/snapcore/snapd/timings.Run()
/home/maciek/work/canonical/snapd/timings/helpers.go:26 +0x84
github.com/snapcore/snapd/overlord/devicestate.(*DeviceManager).populateStateFromSeedImpl()
/home/maciek/work/canonical/snapd/overlord/devicestate/firstboot.go:169 +0x40c
github.com/snapcore/snapd/overlord/devicestate.(*DeviceManager).populateStateFromSeedImpl-fm()
<autogenerated>:1 +0x59
github.com/snapcore/snapd/overlord/devicestate.(*DeviceManager).ensureSeeded.func1()
/home/maciek/work/canonical/snapd/overlord/devicestate/devicemgr.go:991 +0x74
```
Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
This PR makes remodels take into account revision constraints from validation sets on the new model. Additionally, snaps that are marked as invalid in validation sets are checked for in the model.
* a/snapasserts: add methods for extracting more information out of ValidationSets type
* o/assertstate: add ValidationSetsFromModel function for extracting a snapasserts.ValidationSets from an asserts.Model
* o/snapstate: prevent installing/updating a snap from a local file that does not match requested revision
* o/devicestate: consider validation sets during remodeling
* tests/nested/manual: add remodel test that downgrades a snap because of a validation set
* tests/nested/manual: add remodel test that fails to remodel because of an invalid snap in a validation set
* tests/nested/manual: extend offline remodel test to also include a validation set
* tests/lib/assertions: fix timestamps on assertions
* asserts: add Key method to ValidationSet and ModelValidationSet
* o/devicestate: use new Key methods
* o/devicestate: maybe enforce validation sets during doSetModel
* o/devicestate: add test for enforcing validation sets in doSetModel
* a/snapasserts: simplify TestCanBePresent with loop
* tests/lib/assertions: add bluez snap to offline remodel test
* o/devicestate: remove done TODO
* o/snapstate: if remodeling, do not install prereq if link-snap task is present
* tests/nested/manual/remodel-offline: extend test to verify that validation sets are accounted for
* Revert "o/snapstate: if remodeling, do not install prereq if link-snap task is present"
This reverts commit 57c7725a2513df51be7ac1c06c492aaed07a6e3b.
This change is independent and will be included in another PR.
* a/snapasserts: add methods for extracting more information out of ValidationSets type
* o/assertstate: add ValidationSetsFromModel function for extracting a snapasserts.ValidationSets from an asserts.Model
* o/devicestate: add test for ValidationSetsConflictError.Is
* a/snapasserts: move methods after New function
* a/snapasserts: add test for ValidationSets.Revisions to verify ValidationSetsConflictError is returned
* o/assertstate: change ValidationSetsFromModel to take in a DeviceContext, rather than a StoreService
* o/assertstate: rename ValidationSetsModelFlags to ValidationSetsModelOptions
* o/devicestate: add type to export_test to make testing simpler
* tests: add details to new spread tests
* asserts: rename ModelValidationSet.Key and ValidationSet.Key to .SequenceName and add unit tests for them
* o/snapstate: update snap revision mismatch error message to be more clear
* o/devicestate: introduce helper for setting ValidationSets on snapstate.RevisionOptions if Revision is set
* o/devicestate: verify the parameters that fakeSequenceStore receives
* o/devicestate: fix revisions not being respected for essential snaps (and add a test for it)
* o/devicestate: extend TestRemodelUC20EssentialSnapsAlreadyInstalledAndLocal to also exercise case where a validation set requires a revision but the currently installed version is unasserted
* s/seedtest: update retrieveSeq to handle unconstrained sequence forming assertions
* a/snapasserts: add ValidationSets.Sets method
* o/assertstate: add deviceContext to ForgetValidationSet function so that change can happen during remodel
* o/devicestate: attempt to handle rollback of validation sets during failed remodel
* overlord: test for replacing conflicting validation sets during remodel
* o/assertstate: update ForgetValidationSet to take in a DeviceContext and to allow for forcing removal even if the validation set is in use by the model
* o/devicestate: roll back validation set changes on remodel failure
* o/devicestate: make sure that validation sets unrelated to the model survive a remodel
* o/devicestate: rename param in installedSnapRevisionChanged
* o/devicestate: rename field newSnapRevision to newRequiredRevision in modelSnapsForRemodel
* o/devicestate: simplify loops in checkForInvalidSnapsInModel
* o/devicestate: compare validation sets using SequenceName methods
* o/devicestate: fail remodel if we attempt to use an unasserted snap as a specific revision
* tests/nested/manual/remodel-offline: fix test to actually use validation set
* o/devicestate: create helper for creating snapstate.RevisionOptions during remodel
* o/devicestate: name param literals for clarity
* o/devicestate: invert logic to eliminate double negative
* o/devicestate: fix missed inversion of logic
* o/assertstate: update comment on ForgetValidationSetOpts.ForceForget
* overlord, o/devicestate: update remodel test to change models that contain the same validation set
* o/assertstate: test ForceForget functionality in ForgetValidationSet
* o/devicestate: rename function newRevisionOptionsForRemodel to revisionOptionsForRemodel
* o/assertstate, o/devicestate, daemon: remove unneeded DeviceContext param from ForgetValidationSet
* o/devicestate: remove println
* o/devicestate: clarify comment in rollback of adding validation sets
* o/devicestate: rename variable in enforceValidationSetsForRemodel
* o/snapstate: clarify error when attempting to install/refresh local snap with different revision than requested
* o/devicestate: naming consistency
* o/devicestate: simplify error when model is missing snap that is required in validation set
* asserts, overlord, o/devicestate: rename SequenceName to SequenceKey and prefix the series to the string that is returned
* o/snapstate,snap: introduce snap.SelfContainedSetPrereqTracker
It is meant to be used when dealing with a self-contained set of snaps, with no
desire to fetch further snaps, so all prerequisites must be present in the set
itself. This applies to first boot seeding and remodeling for example.
* many: use snap.SelfContainedSetPrereqTracker
also in snap.ValidateBasesAndProviders
these now can produce warnings, OTOH the relaxed checks allow to build/seed an
image even if a content requirement is fulfilled by an alternative provider
notice that with the relaxed checks seeding might fail or the system not work
if the right auto-connections or connections are not in-place
snaps used as default-providers usually have been taken care of already but it
might be up to the user to ask to set that up for alternative ones
* snap: clarify some names
* many: rename and clarify to PrereqTracker.MissingProviderContentTags
* seed/seedwriter: clarify TODO
---------
Co-authored-by: Ernest Lotter <ernest.lotter@canonical.com>
this is a preparatory step to fix the image building side of the issue of not supporting when a default-provider content is fulfilled by an alternative snap
this also drops some tests and behavior related to core16, as it's not pursued anymore and it was getting in the way here
* seed/seedwriter: drop tests about core16 as we are not pursuing it
and are in the way of the changes I'm planning
* seed/seedwriter,image: use snap.ValidateBasesAndProviders for prereq checks
this will be user to adapt to do the right thing when a default provider
is omitted but the content is fulfilled by another snap
* seed/seedwriter: drop some now unused code
* seed/seedwriter: clarify checkPrereqs with comment
also remove spurious repeated call
thanks @alfonsosanchezbeato
* seed/seedwriter: expand the comment in checkPrereqsInMode
Replace ioutil.WriteFile with os.WriteFile since the former has been
deprecated since go1.16 and simply calls the latter.
Signed-off-by: Miguel Pires <miguel.pires@canonical.com>
* asserts/model: add preseedAuthority field to Model
* seed20: allow authority-id to differ from the brand-id
* fixup! asserts/model: add preseedAuthority field to Model
fix comment wording to PreseedAuthority
* fixup! seed20: allow authority-id to differ from the brand-id
clarify error message as "preseed authority-id"
* fixup! asserts/model: add preseedAuthority field to Model
standardize checkOptionalAuthority() signature and make acceptsAny bool explicit when invoking it
* fixup! seed20: allow authority-id to differ from the brand-id
fix ineffectual assignment to preseedAs2
* fixup! asserts/model: add preseedAuthority field to Model
bump copyright years for files touched by 5593e76312
* fixup! seed20: allow authority-id to differ from the brand-id
bump copyright years for files touched by ce7ba34e0f
* fixup! asserts/model: add preseedAuthority field to Model
asserts/model.go: rename "acceptsAny" to "acceptsWildcard"
* seed/seedwriter: produce validation-sets in the manifest
* seed/seedwriter: fix typo in comment
* seed/seedwriter: do not ignore error from MarkValidationSetSeeded
* o/devicestate: add initial code for tracking validation-sets on first boot
* o/devicestate: add possibility for setting validation sets in model setup for firstboot20 tests
* o/devicestate: allow enforcing validation-sets before device is marked seeded, add unit tests to verify tracking completes.
* o/devicestate: restore preseed done task order
* o/devicestate,o/snapstate: create a Local variant of EnforceValidationSets and parameterize enforce-validation-sets task
* overlord: add clarifying comments for the local parameter to "enforce-validation-sets"
* o/assertstate,o/devicestate: add unit tests for ApplyLocalEnforcedValidationSets and firstboot UC18
* o/devicestate: fix wrong comment
* o/devicestate: fix static check for typo
* o/devicestate: add additional unit test checks
* o/snapstate,o/devicestate: review feedback, additional unit tests still needed for handler changes
* overlord: add handler unit tests, add more checks for task data being passed in the firstboot tests
* o/snapstate: add parameter checks for the DoEnforceValidationSetsTask handler
* seed/seedwriter: code for using validation set sequence and pinning from manifest instead of model
* seed/seedwriter: use sequence 2 instead of 100
* seed/seedwriter: simplify the refresh procedure to avoid any sequencekey magic
* seed/seedwriter: do not allow the manifest to override pinned validation-sets
* seed/seedwriter: review feedback and improvements to how we get the final validation set version
* seed/seedwriter: review feedback, do not allow the manifest to change pinning from model settings
* image: review feedback
add unit test case for the case where we end up downloading a different revision than we requested
* image: fix merge gone wrong
* image,seed/seedwriter: integrate SeedManifest into seedwriter.Writer
* seed/seedwriter: add a docstring to writer.SeedManifest()
* image,seed/seedwriter: fix uses of SeedManifest after rename
* seed/seedwriter: review feedback
don't copy ManifestPath as we already store a copy of options, but do keep a new instance of Manifest itself as we may initialize a new copy, fix docstring typo
move initManifest function to Options as a member function. Add seedwriter specific unit tests for manifest functionality.
rename initManifest() to manifest()
* image: review feedback
add unit test case for the case where we end up downloading a different revision than we requested
* image: fix merge gone wrong
* multiple: rename SeedManifest to Manifest for most references, unless it makes sense in the context to keep it as SeedManifest
* seed/seedwriter: rename tests from SeedManifest to Manifest
* multiple: review feedback
rename NewManifestForTest to MockManifest
* image: review feedback
add unit test case for the case where we end up downloading a different revision than we requested
* multiple: move SeedManifest into the seedwriter package.
* image: fix merge gone wrong
* asserts,seed/seedwriter: support for validation sets when writing the image seed.
* seed/seedwriter: simplify writer code
* seed/seedwriter: updatethe docs for Start
* asserts,seed/seedwriter: review feedback
Add unit tests for AtSequence, ensure CheckValidationSets cannot be called unless download step has completed
* seed/seedwriter: review feedback
add unit test for CheckValidationSets to early call, reuse checkStepCompleted in checkSnapsAccessor
* asserts,seed/seedwriter: refactor RefAssertsFetcher into a SeedAssertionFetcher with support for FetchSequence, and its own set of unit tests.
* seed/seedwriter: review feedback
Removed the TODO and updated docs for Writer.Start
* seed/seedwriter: review feedback
remove a line of the docs for SeedAssertionFetcher
* seed/seedwriter: review feedback
add additional case for calling FetchSequence with the default fetcher
