add tests

Author: amohoste

cri/firecracker/coordinator.go

@@ -28,7 +28,7 @@ import (
 	"github.com/ease-lab/vhive/ctriface"
 	"github.com/ease-lab/vhive/metrics"
 	"github.com/ease-lab/vhive/snapshotting"
-	"github.com/ease-lab/vhive/snapshotting/deduplicated"
+	"github.com/ease-lab/vhive/snapshotting/fulllocal"
 	"github.com/ease-lab/vhive/snapshotting/regular"
 	"github.com/pkg/errors"
 	"strconv"
@@ -77,9 +77,9 @@ func newFirecrackerCoordinator(
 	}
 
 	if isFullLocal {
-		c.snapshotManager = snapshotting.NewSnapshotManager(deduplicated.NewSnapshotManager(snapshotsDir, snapsCapacityMiB))
+		c.snapshotManager = snapshotting.NewSnapshotManager(fulllocal.NewSnapshotManager(snapshotsDir, snapsCapacityMiB))
 	} else {
-		c.snapshotManager = snapshotting.NewSnapshotManager(regular.NewRegularSnapshotManager(snapshotsDir))
+		c.snapshotManager = snapshotting.NewSnapshotManager(regular.NewSnapshotManager(snapshotsDir))
 	}
 
 	for _, opt := range opts {
@@ -142,6 +142,7 @@ func (c *coordinator) stopVM(ctx context.Context, containerID string) error {
 	}
 
 	if funcInst.snapBooted {
+		// Release snapshot after the VM has been stopped / offloaded
 		defer c.snapshotManager.ReleaseSnapshot(id)
 	} else {
 		// Create snapshot
@@ -273,7 +274,7 @@ func (c *coordinator) orchCreateSnapshot(ctx context.Context, funcInst *FuncInst
 		id = funcInst.revisionId
 	}
 
-	removeContainerSnaps, snap, err := c.snapshotManager.InitSnapshot(
+	_, snap, err := c.snapshotManager.InitSnapshot(
 		id,
 		funcInst.image,
 		funcInst.coldStartTimeMs,
@@ -286,13 +287,14 @@ func (c *coordinator) orchCreateSnapshot(ctx context.Context, funcInst *FuncInst
 		return nil
 	}
 
-	if c.isFullLocal && removeContainerSnaps != nil {
+	// This call is only necessary if the alternative approach in devicemapper with thin-delta is used.
+	/*if c.isFullLocal && removeContainerSnaps != nil {
 		for _, cleanupSnapId := range *removeContainerSnaps {
 			if err := c.orch.CleanupSnapshot(ctx, cleanupSnapId); err != nil {
 				return errors.Wrap(err, "removing devmapper revision snapshot")
 			}
 		}
-	}
+	}*/
 
 	ctxTimeout, cancel := context.WithTimeout(ctx, time.Second*60)
 	defer cancel()

ctriface/iface.go

@@ -641,6 +641,8 @@ func (o *Orchestrator) OffloadVM(ctx context.Context, vmID string, isFullLocal b
 	return nil
 }
 
+// CleanupSnapshot removes a devicemapper snapshot. This function is only necessary if the alternative approach with
+// thin-delta is used. Otherwise, snapshots created from within vHive get already cleaned up during stopVM.
 func (o *Orchestrator) CleanupSnapshot(ctx context.Context, revisionID string) error {
 	if err := o.devMapper.RemoveDeviceSnapshot(ctx, revisionID); err != nil {
 		return errors.Wrapf(err, "removing revision snapshot")

ctriface/image/manager_test.go

@@ -60,7 +60,6 @@ func TestMain(m *testing.M) {
 		TimestampFormat: ctrdlog.RFC3339NanoFixed,
 		FullTimestamp:   true,
 	})
-	//log.SetReportCaller(true) // FIXME: make sure it's false unless debugging
 
 	log.SetOutput(os.Stdout)
 

devmapper/devicemapper.go

@@ -38,6 +38,12 @@ import (
 	"sync"
 )
 
+// Own managed snapshots in snapmanager and in containerd identified by snapkey. Has name deviceName.
+// snapshotId used internally by containerd, needed for thin_delta. Once committed, snapshots identified
+// by snapName within containerd?
+
+// Only remove snapshots created through createSnapshot. Use key used there to delete them.
+
 // DeviceMapper creates and manages device snapshots used to store container images.
 type DeviceMapper struct {
 	sync.Mutex
@@ -178,6 +184,7 @@ func (dmpr *DeviceMapper) GetDeviceSnapshot(ctx context.Context, snapKey string)
 	_, present := dmpr.snapDevices[snapKey]
 
 	if !present {
+		// Get snapshot from containerd if not yet stored by vHive devicemapper
 		info, err := dmpr.snapshotService.Stat(ctx, snapKey)
 		if err != nil {
 			return nil, err
@@ -210,7 +217,6 @@ func getDeviceName(poolName, snapshotId string) string {
 	return fmt.Sprintf("%s-snap-%s", poolName, snapshotId)
 }
 
-// CreatePatch creates a patch file storing the difference between an image and the container filesystem
 // CreatePatch creates a patch file storing the file differences between and image and the changes applied
 // by the container using rsync. Note that this is a different approach than using thin_delta which is able to
 // extract blocks directly by leveraging the metadata stored by the device mapper.

snapshotting/Makefile

@@ -0,0 +1,33 @@
+# MIT License
+#
+# Copyright (c) 2020 Dmitrii Ustiugov, Plamen Petrov and EASE lab
+#
+# Permission is hereby granted, free of charge, to any person obtaining a copy
+# of this software and associated documentation files (the "Software"), to deal
+# in the Software without restriction, including without limitation the rights
+# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+# copies of the Software, and to permit persons to whom the Software is
+# furnished to do so, subject to the following conditions:
+#
+# The above copyright notice and this permission notice shall be included in all
+# copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+# SOFTWARE.
+
+EXTRAGOARGS:=-v -race -cover
+
+test:
+	# Need to pass GOROOT because GitHub-hosted runners may have several
+	# go versions installed so that calling go from root may fail
+	sudo env "PATH=$(PATH)" "GOROOT=$(GOROOT)" go test ./ $(EXTRAGOARGS)
+
+test-man:
+	echo "Nothing to test manually"
+
+.PHONY: test test-man

snapshotting/deduplicated/manager.go renamed to snapshotting/fulllocal/manager.go

@@ -20,7 +20,7 @@
 // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 // SOFTWARE.
 
-package deduplicated
+package fulllocal
 
 import (
 	"container/heap"
@@ -32,24 +32,27 @@ import (
 	"sync"
 )
 
-// ImprovedSnapshotManager manages snapshots stored on the node.
-type ImprovedSnapshotManager struct {
+// FullLocalSnapshotManager manages snapshots stored on the node.
+type FullLocalSnapshotManager struct {
 	sync.Mutex
+	baseFolder string
+
+	// Stored snapshots
 	snapshots          map[string]*snapshotting.Snapshot
+	// Eviction metadata for stored snapshots
 	snapStats          map[string]*SnapshotStats
 
-	// Heap of snapshots not in use sorted on score
+	// Heap of snapshots not in use that can be freed to save space. Sorted by score
 	freeSnaps  SnapHeap
-	baseFolder string
 
 	// Eviction
 	clock       int64 	// When container last used. Increased to priority terminated container on termination
 	capacityMib int64
 	usedMib     int64
 }
 
-func NewSnapshotManager(baseFolder string, capacityMib int64) *ImprovedSnapshotManager {
-	manager := new(ImprovedSnapshotManager)
+func NewSnapshotManager(baseFolder string, capacityMib int64) *FullLocalSnapshotManager {
+	manager := new(FullLocalSnapshotManager)
 	manager.snapshots = make(map[string]*snapshotting.Snapshot)
 	manager.snapStats = make(map[string]*SnapshotStats)
 	heap.Init(&manager.freeSnaps)
@@ -67,7 +70,7 @@ func NewSnapshotManager(baseFolder string, capacityMib int64) *ImprovedSnapshotM
 
 // AcquireSnapshot returns a snapshot for the specified revision if it is available and increments the internal counter
 // such that the snapshot can't get removed. Similar to how a RW lock works
-func (mgr *ImprovedSnapshotManager) AcquireSnapshot(revision string) (*snapshotting.Snapshot, error) {
+func (mgr *FullLocalSnapshotManager) AcquireSnapshot(revision string) (*snapshotting.Snapshot, error) {
 	mgr.Lock()
 	defer mgr.Unlock()
 
@@ -105,7 +108,7 @@ func (mgr *ImprovedSnapshotManager) AcquireSnapshot(revision string) (*snapshott
 
 // ReleaseSnapshot releases the snapshot with the given revision so that it can possibly get deleted if it is not in use
 // by any other VMs.
-func (mgr *ImprovedSnapshotManager) ReleaseSnapshot(revision string) error {
+func (mgr *FullLocalSnapshotManager) ReleaseSnapshot(revision string) error {
 	mgr.Lock()
 	defer mgr.Unlock()
 
@@ -114,6 +117,10 @@ func (mgr *ImprovedSnapshotManager) ReleaseSnapshot(revision string) error {
 		return errors.New(fmt.Sprintf("Get: Snapshot for revision %s does not exist", revision))
 	}
 
+	if snapStat.numUsing == 0 {
+		return errors.New("Can't release a snapshot that is not in use")
+	}
+
 	snapStat.numUsing -= 1
 
 	if snapStat.numUsing == 0 {
@@ -125,9 +132,9 @@ func (mgr *ImprovedSnapshotManager) ReleaseSnapshot(revision string) error {
 	return nil
 }
 
-// InitSnapshot initializes a snapshot by adding its metadata to the ImprovedSnapshotManager. Once the snapshot has been created,
+// InitSnapshot initializes a snapshot by adding its metadata to the FullLocalSnapshotManager. Once the snapshot has been created,
 // CommitSnapshot must be run to finalize the snapshot creation and make the snapshot available fo ruse
-func (mgr *ImprovedSnapshotManager) InitSnapshot(revision, image string, coldStartTimeMs int64, memSizeMib, vCPUCount uint32, sparse bool) (*[]string, *snapshotting.Snapshot, error) {
+func (mgr *FullLocalSnapshotManager) InitSnapshot(revision, image string, coldStartTimeMs int64, memSizeMib, vCPUCount uint32, sparse bool) (*[]string, *snapshotting.Snapshot, error) {
 	mgr.Lock()
 
 	if _, present := mgr.snapshots[revision]; present {
@@ -172,13 +179,19 @@ func (mgr *ImprovedSnapshotManager) InitSnapshot(revision, image string, coldSta
 }
 
 // CommitSnapshot finalizes the snapshot creation and makes it available for use.
-func (mgr *ImprovedSnapshotManager) CommitSnapshot(revision string) error {
+func (mgr *FullLocalSnapshotManager) CommitSnapshot(revision string) error {
 	mgr.Lock()
 	snapStat, present := mgr.snapStats[revision]
 	if !present {
 		mgr.Unlock()
 		return errors.New(fmt.Sprintf("Snapshot for revision %s to commit does not exist", revision))
 	}
+
+	if snapStat.usable {
+		mgr.Unlock()
+		return errors.New(fmt.Sprintf("Snapshot for revision %s has already been committed", revision))
+	}
+
 	snap := mgr.snapshots[revision]
 	mgr.Unlock()
 
@@ -201,7 +214,7 @@ func (mgr *ImprovedSnapshotManager) CommitSnapshot(revision string) error {
 
 // freeSpace makes sure neededMib of disk space is available by removing unused snapshots. Make sure to have a lock
 // when calling this function.
-func (mgr *ImprovedSnapshotManager) freeSpace(neededMib int64) (*[]string, error) {
+func (mgr *FullLocalSnapshotManager) freeSpace(neededMib int64) (*[]string, error) {
 	var toDelete []string
 	var freedMib int64 = 0
 	var removeContainerSnaps []string
@@ -213,7 +226,7 @@ func (mgr *ImprovedSnapshotManager) freeSpace(neededMib int64) (*[]string, error
 		toDelete = append(toDelete, snapStat.revisionId)
 
 		snap := mgr.snapshots[snapStat.revisionId]
-		removeContainerSnaps = append(removeContainerSnaps, snap.ContainerSnapName)
+		removeContainerSnaps = append(removeContainerSnaps, snap.GetContainerSnapName())
 		freedMib += snapStat.TotalSizeMiB
 	}
 

snapshotting/deduplicated/snapHeap.go renamed to snapshotting/fulllocal/snapHeap.go

@@ -20,7 +20,7 @@
 // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 // SOFTWARE.
 
-package deduplicated
+package fulllocal
 
 type SnapHeap []*SnapshotStats
 

snapshotting/deduplicated/snapStats.go renamed to snapshotting/fulllocal/snapStats.go

@@ -1,7 +1,6 @@
-package deduplicated
+package fulllocal
 
-// Snapshot identified by revision
-// Only capitalized fields are serialised / deserialised
+// SnapshotStats contains snapshot data used by the snapshot manager for its keepalive policy.
 type SnapshotStats struct {
 	revisionId             string