Merge pull request #10220 from bboozzoo/bboozzoo/seedwriter-fail-when…

…-dir-is-present

seed/seedwriter: fail early when system seed directory exists

overlord/devicestate/systems_test.go

@@ -460,18 +460,15 @@ func (s *createSystemSuite) TestCreateSystemWithSomeSnapsAlreadyExisting(c *C) {
 		filepath.Join(unassertedSnapsDir, "other-unasserted_1.0.snap"), 0)
 	c.Assert(err, IsNil)
 
-	// when a given snap in asserted snaps directory already exists, it is
-	// not copied over
+	// the unasserted snap goes into the snaps directory under the system
+	// directory, which triggers the error in creating the directory by
+	// seed writer
 	newFiles, dir, err = devicestate.CreateSystemForModelFromValidatedSnaps(modelWithUnasserted, "1234unasserted", s.db, infoGetter)
 
-	c.Assert(err, ErrorMatches, "unable to create .*/other-unasserted_1.0.snap: file exists")
-	c.Check(newFiles, DeepEquals, []string{
-		// returned as new file so that cleanup can be properly
-		// performed even if the file was partially written
-		filepath.Join(unassertedSnapsDir, "other-unasserted_1.0.snap"),
-	})
-	c.Check(dir, Equals, filepath.Join(boot.InitramfsUbuntuSeedDir, "systems/1234unasserted"))
-	c.Check(osutil.IsDirectory(filepath.Join(boot.InitramfsUbuntuSeedDir, "systems/1234unasserted")), Equals, true)
+	c.Assert(err, ErrorMatches, `system "1234unasserted" already exists`)
+	// we failed early
+	c.Check(newFiles, HasLen, 0)
+	c.Check(dir, Equals, "")
 }
 
 func (s *createSystemSuite) TestCreateSystemInfoAndAssertsChecks(c *C) {

seed/seedwriter/seed20.go

@@ -159,7 +159,18 @@ func (tr *tree20) mkFixedDirs() error {
 		return err
 	}
 
-	return os.MkdirAll(tr.systemDir, 0755)
+	if err := os.MkdirAll(filepath.Dir(tr.systemDir), 0755); err != nil {
+		return err
+	}
+	if err := os.Mkdir(tr.systemDir, 0755); err != nil {
+		if os.IsExist(err) {
+			return &SystemAlreadyExistsError{
+				label: tr.opts.Label,
+			}
+		}
+		return err
+	}
+	return nil
 }
 
 func (tr *tree20) ensureSystemSnapsDir() (string, error) {

seed/seedwriter/writer.go

@@ -421,12 +421,28 @@ func (w *Writer) SetOptionsSnaps(optSnaps []*OptionsSnap) error {
 	return nil
 }
 
+// SystemAlreadyExistsError is an error returned when given seed system already
+// exists.
+type SystemAlreadyExistsError struct {
+	label string
+}
+
+func (e *SystemAlreadyExistsError) Error() string {
+	return fmt.Sprintf("system %q already exists", e.label)
+}
+
+func IsSytemDirectoryExistsError(err error) bool {
+	_, ok := err.(*SystemAlreadyExistsError)
+	return ok
+}
+
 // Start starts the seed writing. It creates a RefAssertsFetcher using
-// newFetcher and uses it to fetch model related assertions. For
-// convenience it returns the fetcher possibly for use to fetch seed
-// snap assertions, a task that the writer delegates as well as snap
-// downloading. The writer assumes that the snap assertions will end up
-// in the given db (writing assertion database).
+// newFetcher and uses it to fetch model related assertions. For convenience it
+// returns the fetcher possibly for use to fetch seed snap assertions, a task
+// that the writer delegates as well as snap downloading. The writer assumes
+// that the snap assertions will end up in the given db (writing assertion
+// database). When the system seed directory is already present,
+// SystemAlreadyExistsError is returned.
 func (w *Writer) Start(db asserts.RODatabase, newFetcher NewFetcherFunc) (RefAssertsFetcher, error) {
 	if err := w.checkStep(startStep); err != nil {
 		return nil, err

seed/seedwriter/writer_test.go

@@ -2156,8 +2156,9 @@ func (s *writerSuite) TestDownloadedCore20CheckBaseCoreXX(c *C) {
 		{[]interface{}{requiredBaseCore16Ent}, `cannot add snap "required-base-core16" without also adding its base "core16" \(or "core"\) explicitly`},
 	}
 
-	s.opts.Label = "20191003"
-	for _, t := range tests {
+	baseLabel := "20191003"
+	for idx, t := range tests {
+		s.opts.Label = fmt.Sprintf("%s%d", baseLabel, idx)
 		snaps := []interface{}{
 			map[string]interface{}{
 				"name":            "pc-kernel",
@@ -2326,7 +2327,7 @@ func (s *writerSuite) TestCore20NonDangerousDisallowedOptionsSnaps(c *C) {
 
 	pcFn := s.makeLocalSnap(c, "pc")
 
-	s.opts.Label = "20191107"
+	baseLabel := "20191107"
 
 	tests := []struct {
 		optSnap *seedwriter.OptionsSnap
@@ -2338,7 +2339,8 @@ func (s *writerSuite) TestCore20NonDangerousDisallowedOptionsSnaps(c *C) {
 
 	const expectedErr = `cannot override channels, add devmode snaps, local snaps, or extra snaps with a model of grade higher than dangerous`
 
-	for _, t := range tests {
+	for idx, t := range tests {
+		s.opts.Label = fmt.Sprintf("%s%d", baseLabel, idx)
 		w, err := seedwriter.New(model, s.opts)
 		c.Assert(err, IsNil)
 
@@ -3201,3 +3203,36 @@ func (s *writerSuite) TestSeedSnapsWriteMetaCore20SignedLocalAssertedSnaps(c *C)
 	// no options file was created
 	c.Check(filepath.Join(systemDir, "options.yaml"), testutil.FileAbsent)
 }
+
+func (s *writerSuite) TestSeedSnapsWriteCore20ErrWhenDirExists(c *C) {
+	model := s.Brands.Model("my-brand", "my-model", map[string]interface{}{
+		"display-name": "my model",
+		"architecture": "amd64",
+		"base":         "core20",
+		"grade":        "signed",
+		"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",
+			}},
+	})
+
+	err := os.MkdirAll(filepath.Join(s.opts.SeedDir, "systems", "1234"), 0755)
+	c.Assert(err, IsNil)
+	s.opts.Label = "1234"
+	w, err := seedwriter.New(model, s.opts)
+	c.Assert(err, IsNil)
+	c.Assert(w, NotNil)
+
+	_, err = w.Start(s.db, s.newFetcher)
+	c.Assert(err, ErrorMatches, `system "1234" already exists`)
+	c.Assert(seedwriter.IsSytemDirectoryExistsError(err), Equals, true)
+}