snapd
snapd copied to clipboard
snapcore
seed/seedwriter: fix issue that base snaps set in option will be ignored
Hi,
When using the following model assertion to build image with command snap prepare-image --channel=stable --snap=core22=latest/stable --snap=network-manager=22/stable --snap=modem-manager=latest/stable --snap=core=latest/stable model.assert work/unpack, I will get this error error: cannot add snap "modem-manager" without also adding its base "core" explicitly
type: model
authority-id: V2yG6LAupEYdeTz6jHQLLOMkHGd1ZG44
revision: 1
series: 16
brand-id: V2yG6LAupEYdeTz6jHQLLOMkHGd1ZG44
model: ubuntu-core-20-amd64-dangerous
architecture: amd64
base: core20
grade: dangerous
snaps:
-
default-channel: 20/edge
id: UqFziVZDHLSyO3TqSWgNBoAdHbLI4dAH
name: pc
type: gadget
-
default-channel: 20/edge
id: pYVQrBcKmBa0mZ4CCN7ExT6jH8rY1hza
name: pc-kernel
type: kernel
-
default-channel: latest/edge
id: DLqre5XGLbDqg9jPtiAhRRjDuPVa5X1q
name: core20
type: base
-
default-channel: latest/edge
id: PMrrV4ml8uWuEUDBT8dSGnKUYbevVhc4
name: snapd
type: snapd
-
default-channel: 20/stable
id: KtwxgRlwCAVKFw92BUdt1WloH1Va3QPo
name: modem-manager
type: app
-
default-channel: 20/stable
id: RmBXKl6HO6YOC2DE4G2q1JzWImC04EUy
name: network-manager
type: app
timestamp: 2023-03-29T11:18:00.0Z
sign-key-sha3-384: J-RYIwjKa7kBS5WpVNM036ri24pNeOaAM3nbDzyv8q55iMF6Y3nJqsVYiBtI56vE
AcLBcwQAAQoAHRYhBBP9k65i7BkdXwGeTA1TXkI+U5x2BQJkO9deAAoJEA1TXkI+U5x20BAQAI3y
mzGeGn8UMD6XDukDG24tErrc/CpHQbEHxaK8HjuSUni2O5XPxLMb8bz1X0rk9PDcWm/fesRysOKE
CLNITvGoL8gpU4qCI7tZ4mVwjfRYfjCgAX4/lKWVxeqgrVgYDmVkfEvApnLulctaIKwCMwHDjy9J
uwmTYkacWPA3aajIW7ArGNPigYgnnq23mPF2ilRbFjGwT1cGf6wxO6OIOJd30Ih+5Fr+6XV6bEr0
PVO+NJGsHlZFm0Hwkc8GwKVbTDVolQodtktBjJ9KdUjlvOgu3Czyh8nol67F81FVUEEzjtvsp4rW
BRzGi9TzvUsQ78rEpSVKwFZQEyHMplw0w4HdZfSxoLte6SDJir/yaeErHHxq4yNdSrfsMJABTb3i
ZhcCJxJZ/3Vls07VUzyN5uSZhVN/G4KeL9Pg8k/MEgva9nLJicPSwe3xs6Cmf9QCcQcvllGH2hQd
5/M1pICViBWEiYKEP0hUTAvCxaZlxTFJIpNxo6TH+zSTJZhzDtd3I53PBLFZNzhzfXx3GGI1t5tY
s4NqAcksf5gvvOocdG3mLZdZtqimCt66tYueKLkxMx1Q3Od6l0vz250Ve28EAPqXF6bNsNl3O2jv
J4VZvfHWRMiydvwhxWkC9geYqo/Gdlo/cj8vmeSa0cscrrOc2f6I+N9MZ9+VcuvdoTWwtLm8
Apparently, I do set the core and core22 snap in option, but they are somehow ignored by snapd. If using the same command with the following model assertion, there is no such a issue
type: model
authority-id: canonical
revision: 1
series: 16
brand-id: canonical
model: ubuntu-core-20-amd64-dangerous
architecture: amd64
base: core20
grade: dangerous
snaps:
-
default-channel: 20/edge
id: UqFziVZDHLSyO3TqSWgNBoAdHbLI4dAH
name: pc
type: gadget
-
default-channel: 20/edge
id: pYVQrBcKmBa0mZ4CCN7ExT6jH8rY1hza
name: pc-kernel
type: kernel
-
default-channel: latest/edge
id: DLqre5XGLbDqg9jPtiAhRRjDuPVa5X1q
name: core20
type: base
-
default-channel: latest/edge
id: PMrrV4ml8uWuEUDBT8dSGnKUYbevVhc4
name: snapd
type: snapd
timestamp: 2020-04-29T11:18:00.0Z
sign-key-sha3-384: 9tydnLa6MTJ-jaQTFUXEwHl1yRx7ZS4K5cyFDhYDcPzhS7uyEkDxdUjg9g08BtNn
AcLBXAQAAQoABgUCXqmLygAKCRDgT5vottzAEjtpD/976igsvImmT56zho47BLbkJYPuC/lgvI+8
84Z9Oi3Oq7oRapm9Fn8AnpKu+0K98XWv4okdjgRrp0fo2ClXcWfQLjMKAtT9dm003hVEjB1IEk1n
SJ2kwmIDvnmFLNA//tteZ5DsAIOUNS6Ug9jEsyMNCgKCKGe2Lprxeukn2VcqrVym1d0chGPsz/VT
Jtwm9WGkUrBqLWj8vfzOaEToqLBDxC7AU5SnrsQpNwgsrnnjFJfY8CBYbcktCC1T7x4Vo+5qEqmA
B99nQPBBHLgT4lA4pQwW/n4h1qJPQvt5SMwtYjPfFpaVSM+Qvs8j4ZBOli0xeP4oIDNJ2qRZ6nkb
hdr2abPWh7wQlmLW04qfJRN7iMBX8vCMWRBlJzFADzfdYHtKs00MSbQoUEtF6mB3Aq84TUdZ40RG
g4ylkYgG5XGAQch3vYwS4LTlqXosXWoPhiMHvrYWlJQBeqyUXD+onK/J1QNM9ff5twH15jy11v9F
ODVTufUV42f70aA6y43cv4Kt8uc0GmHEmMhwYq3NBiOZuLEjKI2IFLJcZchYsZck3/NLMbVi2KrN
aMzgJqkgefM4WbuOpVR+PhfMx5vTnL7b4IFMH6z4tsXKkIRxX+wUgrSIhjE1Dpl4jnHuQM2pF2oq
pbrXk+N/WD0PgoPFDXilYqpqj3kw0B/jVUiWhUZXBA==
Thanks for spending time review this PR, it may not be the best solution due to my lack of understanding of the whole mechanism, feel free to give me any suggestions, thanks!
Sorry that I am still quite new to golang, I have tried to run the test case with command go test -v ./seed/seedwriter/, but I will get the following error, not sure if I did anything incorrectly
$ go test -v ./seed/seedwriter/
# github.com/snapcore/secboot/tpm2
../../go/pkg/mod/github.com/snapcore/[email protected]/tpm2/pcr_profile.go:1127:12: assignment mismatch: 2 variables but values[0].SelectionList returns 1 values
FAIL github.com/snapcore/snapd/seed/seedwriter [build failed]
FAIL
Hi,
Thanks for the review! I have rebased the code, and also run go fmt to remove the additional whitespace that you have found previously.
I am now able to run the go test, however, looks like my code breaks one of the test cases, I will find some time to see if I can fix it
$ go test -v ./seed/seedwriter/
=== RUN Test
----------------------------------------------------------------------
FAIL: writer_test.go:2077: writerSuite.TestSeedSnapsWriteMetaLocalExtraSnaps
writer_test.go:2146:
c.Assert(complete, Equals, false)
... obtained bool = true
... expected bool = false
OOPS: 92 passed, 1 FAILED
--- FAIL: Test (1.90s)
FAIL
FAIL github.com/snapcore/snapd/seed/seedwriter 1.945s
FAIL
Codecov Report
Attention: Patch coverage is 33.33333% with 6 lines in your changes are missing coverage. Please review.
Project coverage is 78.92%. Comparing base (
b1f86be) to head (5e54bd4).
| Files | Patch % | Lines |
|---|---|---|
| seed/seedwriter/seed20.go | 14.28% | 5 Missing and 1 partial :warning: |
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@ Coverage Diff @@
## master #12745 +/- ##
==========================================
- Coverage 78.92% 78.92% -0.01%
==========================================
Files 1043 1043
Lines 134425 134431 +6
==========================================
+ Hits 106091 106093 +2
- Misses 21721 21725 +4
Partials 6613 6613
| Flag | Coverage Δ | |
|---|---|---|
| unittests | 78.92% <33.33%> (-0.01%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hi snapd team,
I have modified the code and it is able to pass all the existing test cases, and it can also solve the issue that I have encountered
I spent some time trying to write a test case, but I encounter some issues that I think it's a bit complicated, so I might need your suggestions. Here is the initial draft of my test case
diff --git a/seed/seedwriter/writer_test.go b/seed/seedwriter/writer_test.go
index 7ff58a9..a64a909 100644
--- a/seed/seedwriter/writer_test.go
+++ b/seed/seedwriter/writer_test.go
@@ -169,6 +169,16 @@ type: app
base: core16
version: 1.0
`,
+ "required-base-core20": `name: required
+type: app
+base: core20
+version: 1.0
+`,
+ "required-base-core22": `name: required
+type: app
+base: core22
+version: 2.0
+`,
"my-devmode": `name: my-devmode
type: app
version: 1
@@ -2500,6 +2510,58 @@ func (s *writerSuite) TestDownloadedCore20CheckBase(c *C) {
c.Check(err, ErrorMatches, `cannot add snap "cont-producer" without also adding its base "core18" explicitly`)
}
+func (s *writerSuite) TestDownloadedCore20CheckBaseAristoTest(c *C) {
+ model := s.Brands.Model("my-brand", "my-model", map[string]interface{}{
+ "display-name": "my model",
+ "architecture": "amd64",
+ "store": "my-store",
+ "base": "core20",
+ "grade": "dangerous",
+ "snaps": []interface{}{
+ map[string]interface{}{
+ "name": "pc-kernel",
+ "id": s.AssertedSnapID("pc-kernel"),
+ "type": "kernel",
+ "default-channel": "20",
+ },
+ map[string]interface{}{
+ "name": "pc",
+ "id": s.AssertedSnapID("pc"),
+ "type": "gadget",
+ "default-channel": "20",
+ },
+ map[string]interface{}{
+ "name": "required",
+ "id": s.AssertedSnapID("required"),
+ "type": "app",
+ "default-channel": "20",
+ },
+ },
+ })
+
+ // validity
+ c.Assert(model.Grade(), Equals, asserts.ModelDangerous)
+
+ s.makeSnap(c, "snapd", "")
+ s.makeSnap(c, "core20", "")
+ s.makeSnap(c, "core22", "")
+ s.makeSnap(c, "pc-kernel=20", "")
+ s.makeSnap(c, "pc=20", "")
+ s.makeSnap(c, "required-base-core20", "")
+ s.makeSnap(c, "required-base-core22", "")
+ //s.MakeAssertedSnap(c, snapYaml["required-base-core20"], snapFiles["required-base-core20"], snap.R(2), "developerid", s.StoreSigning.Database)
+ //s.MakeAssertedSnap(c, snapYaml["required-base-core22"], snapFiles["required-base-core22"], snap.R(3), "developerid", s.StoreSigning.Database)
+ s.opts.Label = "20231002"
+ //complete, w, err := s.upToDownloaded(c, model, s.fillDownloadedSnap, s.fetchAsserts(c), &seedwriter.OptionsSnap{Name: "required-base-core22"}, &seedwriter.OptionsSnap{Name: "core22"})
+ complete, w, err := s.upToDownloaded(c, model, s.fillDownloadedSnap, s.fetchAsserts(c), &seedwriter.OptionsSnap{Name: "required-base-core22"})
+ c.Assert(err, IsNil)
+ c.Check(complete, Equals, false)
+
+ snaps, err := w.SnapsToDownload()
+ c.Assert(err, IsNil)
+ c.Assert(snaps, HasLen, 2)
+}
+
func (s *writerSuite) TestDownloadedCore20CheckBaseModes(c *C) {
model := s.Brands.Model("my-brand", "my-model", map[string]interface{}{
"display-name": "my model",
I will get this error
FAIL: writer_test.go:2513: writerSuite.TestDownloadedCore20CheckBaseAristoTest
writer_test.go:2551:
s.makeSnap(c, "required-base-core22", "")
/home/vagrant/aristo/snapd-fix-options-snaps-base-checking/seed/seedtest/seedtest.go:136:
c.Assert(err, IsNil)
... value *asserts.RevisionError = &asserts.RevisionError{Used:0, Current:0} ("revision 0 is already the current revision")
If I understand correctly, it means that the assertion revision is already used. Looks like this error is related to the mechanism of assertion assembling, signing, adding to db, etc. So currently I am wondering maybe need a mechanism to
- check if revision exists in db
- if yes, then add the biggest revision num by 1, otherwise, use 0
But I am not familiar with the whole mechanism of the code for assertion, so I would prefer to discuss first before I start implementing
[Other info (Probably not important)] I tried to comment out the following code, and my test case mentioned above works as I expected
https://github.com/snapcore/snapd/blob/558a947ac83723929e828a08c695b25c72736649/seed/seedtest/seedtest.go#L132
Thanks! Aristo
@AristoChen
snap prepare-image --channel=stable --snap=core22=latest/stable --snap=network-manager=22/stable --snap=modem-manager=latest/stable --snap=core=latest/stable model.assert work/unpack
I don't understand what is the intention here, UC20+ models are meant to be self-contained and explicit (except for the snapd snap that can be left implicit), which means snaps cannot be switched around to different tracks if it implies getting bases not in the model
Hi @pedronis ,
Previously I was debugging network-manager with track 20(which is defined in my model assertion) for a DNS issue, and then I decided to do a quick test whether the network-manager from track 22 has the same issue or not, if there is no issue with 22 track, then probably only need to backport some patches to 20 track
so the intention is that I would like to switch to different track of network-manager to test without requesting the customer to sign a new model assertion for me
@AristoChen we recently changed how/when we check for bases in seedwriter, it's possible that is now works with a snap prepare-image using snapd from edge if and only if the model is grade dangerous, maybe you could try
Hi @pedronis ,
I tried with latest/edge(rev 20722), and I still have the same issue
I finally got to write a reproducer: https://github.com/pedronis/snappy/commit/a40c496c9f1db2bb6009d66c7ed44c47ca644970 . This fails also on master, the issue that I missed is that modem-manager is a required snap but we are asking to use a channel where it has a different base, and also supplying that as a option snap. I need to see what makes sense to do with the changes happened in master since.