Completely serialize creation

...to fix races between parallel loop of creations and cleanup. [finishes #132206493] Signed-off-by: George Lestaris <[email protected]>
cloudfoundry · Oct 26, 2016 · f36efc7 · f36efc7
1 parent c36c9ee
commit f36efc7
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 46 deletions.
diff --git a/groot/creator.go b/groot/creator.go
@@ -63,20 +63,17 @@ func (c *Creator) Create(logger lager.Logger, spec CreateSpec) (Bundle, error) {
 	if err != nil {
 		return Bundle{}, err
 	}
-
-	image, err := c.imagePuller.Pull(logger, imageSpec)
-	if err != nil {
+	defer func() {
 		if err := c.locksmith.Unlock(lockFile); err != nil {
 			logger.Error("failed-to-unlock", err)
 		}
+	}()
 
+	image, err := c.imagePuller.Pull(logger, imageSpec)
+	if err != nil {
 		return Bundle{}, errorspkg.Wrap(err, "pulling the image")
 	}
 
-	if err := c.locksmith.Unlock(lockFile); err != nil {
-		logger.Error("failed-to-unlock", err)
-	}
-
 	bundleSpec := BundleSpec{
 		ID:                    spec.ID,
 		DiskLimit:             spec.DiskLimit,

diff --git a/groot/creator_test.go b/groot/creator_test.go
@@ -78,16 +78,6 @@ var _ = Describe("Creator", func() {
 			Expect(imageSpec.GIDMappings).To(Equal(gidMappings))
 		})
 
-		It("releases the global lock", func() {
-			_, err := creator.Create(logger, groot.CreateSpec{
-				Image: "/path/to/image",
-			})
-			Expect(err).NotTo(HaveOccurred())
-
-			Expect(fakeLocksmith.UnlockCallCount()).To(Equal(1))
-			Expect(fakeLocksmith.UnlockArgsForCall(0)).To(Equal(lockFile))
-		})
-
 		It("makes a bundle", func() {
 			image := groot.Image{
 				VolumePath: "/path/to/volume",
@@ -132,6 +122,16 @@ var _ = Describe("Creator", func() {
 			Expect(chainIDs).To(Equal([]string{"sha256:vol-a", "sha256:vol-b"}))
 		})
 
+		It("releases the global lock", func() {
+			_, err := creator.Create(logger, groot.CreateSpec{
+				Image: "/path/to/image",
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			Expect(fakeLocksmith.UnlockCallCount()).To(Equal(1))
+			Expect(fakeLocksmith.UnlockArgsForCall(0)).To(Equal(lockFile))
+		})
+
 		It("returns the bundle", func() {
 			fakeBundler.CreateReturns(groot.Bundle{
 				Path: "/path/to/bundle",
@@ -256,16 +256,6 @@ var _ = Describe("Creator", func() {
 
 				Expect(fakeBundler.CreateCallCount()).To(Equal(0))
 			})
-
-			It("releases the global lock", func() {
-				_, err := creator.Create(logger, groot.CreateSpec{
-					Image: "/path/to/image",
-				})
-				Expect(err).To(HaveOccurred())
-
-				Expect(fakeLocksmith.UnlockCallCount()).To(Equal(1))
-				Expect(fakeLocksmith.UnlockArgsForCall(0)).To(Equal(lockFile))
-			})
 		})
 
 		Context("when creating the bundle fails", func() {
@@ -306,7 +296,7 @@ var _ = Describe("Creator", func() {
 		})
 
 		Context("when disk limit is given", func() {
-			It("applies the disk limit", func() {
+			It("passes the disk limit to the bundler", func() {
 				image := groot.Image{
 					VolumePath: "/path/to/volume",
 					Image: specsv1.Image{

diff --git a/integration/groot/create_concurrency_test.go b/integration/groot/create_concurrency_test.go
@@ -2,33 +2,25 @@ package groot_test
 
 import (
 	"fmt"
-	"io/ioutil"
-	"os"
 	"sync"
 
-	"code.cloudfoundry.org/grootfs/integration"
+	"code.cloudfoundry.org/grootfs/groot"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 )
 
 var _ = Describe("Concurrent creations", func() {
-	var imagePath string
-
 	BeforeEach(func() {
-		var err error
-		imagePath, err = ioutil.TempDir("", "")
+		// run this to setup the store before concurrency!
+		_, err := Runner.Create(groot.CreateSpec{
+			ID:    "test-pre-warm",
+			Image: "docker:///cfgarden/empty",
+		})
 		Expect(err).NotTo(HaveOccurred())
 	})
 
-	AfterEach(func() {
-		Expect(os.RemoveAll(imagePath)).To(Succeed())
-	})
-
 	It("can create multiple rootfses of the same image concurrently", func() {
-		// run this to setup the store before concurrency!
-		integration.CreateBundle(GrootFSBin, StorePath, DraxBin, imagePath, "test-pre-warm", 0)
-
 		wg := new(sync.WaitGroup)
 
 		for i := 0; i < 3; i++ {
@@ -37,13 +29,44 @@ var _ = Describe("Concurrent creations", func() {
 				defer GinkgoRecover()
 				defer wg.Done()
 
-				integration.CreateBundle(
-					GrootFSBin, StorePath, DraxBin, "docker:///cfgarden/empty",
-					fmt.Sprintf("test-%d", idx), 0,
-				)
+				_, err := Runner.Create(groot.CreateSpec{
+					ID:    fmt.Sprintf("test-%d", idx),
+					Image: "docker:///cfgarden/empty",
+				})
+				Expect(err).NotTo(HaveOccurred())
 			}(wg, i)
 		}
 
 		wg.Wait()
 	})
+
+	Describe("parallel create and clean", func() {
+		It("works in parallel, without errors", func() {
+			wg := new(sync.WaitGroup)
+
+			wg.Add(1)
+			go func() {
+				defer GinkgoRecover()
+				defer wg.Done()
+
+				for i := 0; i < 3; i++ {
+					_, err := Runner.Create(groot.CreateSpec{
+						ID:    fmt.Sprintf("test-%d", i),
+						Image: "docker:///cfgarden/empty",
+					})
+					Expect(err).NotTo(HaveOccurred())
+				}
+			}()
+
+			wg.Add(1)
+			go func() {
+				defer GinkgoRecover()
+				defer wg.Done()
+
+				Expect(Runner.Clean(0)).To(Succeed())
+			}()
+
+			wg.Wait()
+		})
+	})
 })