Address PR remarks

Signed-off-by: Amory Hoste <[email protected]>

Author: amohoste

cri/firecracker/coordinator.go

@@ -62,7 +62,13 @@ func withoutOrchestrator() coordinatorOption {
 	}
 }
 
-func newFirecrackerCoordinator(orch *ctriface.Orchestrator, snapsCapacityMiB int64, isSparseSnaps bool, isFullLocal bool, opts ...coordinatorOption) *coordinator {
+func newFirecrackerCoordinator(
+	orch *ctriface.Orchestrator,
+	snapsCapacityMiB int64,
+	isSparseSnaps bool,
+	isFullLocal bool,
+	opts ...coordinatorOption) *coordinator {
+
 	c := &coordinator{
 		activeInstances: make(map[string]*FuncInstance),
 		orch:            orch,
@@ -114,7 +120,7 @@ func (c *coordinator) startVM(ctx context.Context, image string, revision string
 func (c *coordinator) stopVM(ctx context.Context, containerID string) error {
 	c.Lock()
 
-	fi, present := c.activeInstances[containerID]
+	funcInst, present := c.activeInstances[containerID]
 	if present {
 		delete(c.activeInstances, containerID)
 	}
@@ -127,28 +133,28 @@ func (c *coordinator) stopVM(ctx context.Context, containerID string) error {
 	}
 
 	if c.orch == nil || ! c.orch.GetSnapshotsEnabled() {
-		return c.orchStopVM(ctx, fi)
+		return c.orchStopVM(ctx, funcInst)
 	}
 
-	id := fi.vmID
+	id := funcInst.vmID
 	if c.isFullLocal {
-		id = fi.revisionId
+		id = funcInst.revisionId
 	}
 
-	if fi.snapBooted {
+	if funcInst.snapBooted {
 		defer c.snapshotManager.ReleaseSnapshot(id)
 	} else {
 		// Create snapshot
-		err := c.orchCreateSnapshot(ctx, fi)
+		err := c.orchCreateSnapshot(ctx, funcInst)
 		if err != nil {
 			log.Printf("Err creating snapshot %s\n", err)
 		}
 	}
 
 	if c.isFullLocal {
-		return c.orchStopVM(ctx, fi)
+		return c.orchStopVM(ctx, funcInst)
 	} else {
-		return c.orchOffloadVM(ctx, fi)
+		return c.orchOffloadVM(ctx, funcInst)
 	}
 }
 
@@ -161,18 +167,18 @@ func (c *coordinator) isActive(containerID string) bool {
 	return ok
 }
 
-func (c *coordinator) insertActive(containerID string, fi *FuncInstance) error {
+func (c *coordinator) insertActive(containerID string, funcInst *FuncInstance) error {
 	c.Lock()
 	defer c.Unlock()
 
-	logger := log.WithFields(log.Fields{"containerID": containerID, "vmID": fi.vmID})
+	logger := log.WithFields(log.Fields{"containerID": containerID, "vmID": funcInst.vmID})
 
 	if fi, present := c.activeInstances[containerID]; present {
 		logger.Errorf("entry for container already exists with vmID %s" + fi.vmID)
 		return errors.New("entry for container already exists")
 	}
 
-	c.activeInstances[containerID] = fi
+	c.activeInstances[containerID] = funcInst
 	return nil
 }
 
@@ -206,12 +212,18 @@ func (c *coordinator) orchStartVM(ctx context.Context, image, revision string, m
 
 	coldStartTimeMs := metrics.ToMs(time.Since(tStartCold))
 
-	fi := NewFuncInstance(vmID, image, revision, resp, false, memSizeMib, vCPUCount, coldStartTimeMs)
+	funcInst := NewFuncInstance(vmID, image, revision, resp, false, memSizeMib, vCPUCount, coldStartTimeMs)
 	logger.Debug("successfully created fresh instance")
-	return fi, err
+	return funcInst, err
 }
 
-func (c *coordinator) orchStartVMSnapshot(ctx context.Context, snap *snapshotting.Snapshot, memSizeMib, vCPUCount uint32, vmID string) (*FuncInstance, error) {
+func (c *coordinator) orchStartVMSnapshot(
+	ctx context.Context,
+	snap *snapshotting.Snapshot,
+	memSizeMib,
+	vCPUCount uint32,
+	vmID string) (*FuncInstance, error) {
+
 	tStartCold := time.Now()
 	logger := log.WithFields(
 		log.Fields{
@@ -242,29 +254,35 @@ func (c *coordinator) orchStartVMSnapshot(ctx context.Context, snap *snapshottin
 	}
 
 	coldStartTimeMs := metrics.ToMs(time.Since(tStartCold))
-	fi := NewFuncInstance(vmID, snap.GetImage(), snap.GetId(), resp, true, memSizeMib, vCPUCount, coldStartTimeMs)
+	funcInst := NewFuncInstance(vmID, snap.GetImage(), snap.GetId(), resp, true, memSizeMib, vCPUCount, coldStartTimeMs)
 	logger.Debug("successfully loaded instance from snapshot")
 
-	return fi, err
+	return funcInst, err
 }
 
-func (c *coordinator) orchCreateSnapshot(ctx context.Context, fi *FuncInstance) error {
+func (c *coordinator) orchCreateSnapshot(ctx context.Context, funcInst *FuncInstance) error {
 	logger := log.WithFields(
 		log.Fields{
-			"vmID":  fi.vmID,
-			"image": fi.image,
+			"vmID":  funcInst.vmID,
+			"image": funcInst.image,
 		},
 	)
 
-	id := fi.vmID
+	id := funcInst.vmID
 	if c.isFullLocal {
-		id = fi.revisionId
+		id = funcInst.revisionId
 	}
 
-	removeContainerSnaps, snap, err := c.snapshotManager.InitSnapshot(id, fi.image, fi.coldStartTimeMs, fi.memSizeMib, fi.vCPUCount, c.isSparseSnaps)
+	removeContainerSnaps, snap, err := c.snapshotManager.InitSnapshot(
+		id,
+		funcInst.image,
+		funcInst.coldStartTimeMs,
+		funcInst.memSizeMib,
+		funcInst.vCPUCount,
+		c.isSparseSnaps)
 
 	if err != nil {
-		fi.logger.Warn(fmt.Sprint(err))
+		funcInst.logger.Warn(fmt.Sprint(err))
 		return nil
 	}
 
@@ -281,46 +299,46 @@ func (c *coordinator) orchCreateSnapshot(ctx context.Context, fi *FuncInstance)
 
 	logger.Debug("creating instance snapshot before stopping")
 
-	err = c.orch.PauseVM(ctxTimeout, fi.vmID)
+	err = c.orch.PauseVM(ctxTimeout, funcInst.vmID)
 	if err != nil {
 		logger.WithError(err).Error("failed to pause VM")
 		return nil
 	}
 
-	err = c.orch.CreateSnapshot(ctxTimeout, fi.vmID, snap, c.isFullLocal)
+	err = c.orch.CreateSnapshot(ctxTimeout, funcInst.vmID, snap, c.isFullLocal)
 	if err != nil {
-		fi.logger.WithError(err).Error("failed to create snapshot")
+		funcInst.logger.WithError(err).Error("failed to create snapshot")
 		return nil
 	}
 
 	if err := c.snapshotManager.CommitSnapshot(id); err != nil {
-		fi.logger.WithError(err).Error("failed to commit snapshot")
+		funcInst.logger.WithError(err).Error("failed to commit snapshot")
 		return err
 	}
 
 	return nil
 }
 
-func (c *coordinator) orchOffloadVM(ctx context.Context, fi *FuncInstance) error {
+func (c *coordinator) orchOffloadVM(ctx context.Context, funcInst *FuncInstance) error {
 	if c.withoutOrchestrator {
 		return nil
 	}
 
-	if err := c.orch.OffloadVM(ctx, fi.vmID, c.isFullLocal); err != nil {
-		fi.logger.WithError(err).Error("failed to offload VM")
+	if err := c.orch.OffloadVM(ctx, funcInst.vmID, c.isFullLocal); err != nil {
+		funcInst.logger.WithError(err).Error("failed to offload VM")
 		return err
 	}
 
 	return nil
 }
 
-func (c *coordinator) orchStopVM(ctx context.Context, fi *FuncInstance) error {
+func (c *coordinator) orchStopVM(ctx context.Context, funcInst *FuncInstance) error {
 	if c.withoutOrchestrator {
 		return nil
 	}
 
-	if err := c.orch.StopSingleVM(ctx, fi.vmID, c.isFullLocal); err != nil {
-		fi.logger.WithError(err).Error("failed to stop VM for instance")
+	if err := c.orch.StopSingleVM(ctx, funcInst.vmID, c.isFullLocal); err != nil {
+		funcInst.logger.WithError(err).Error("failed to stop VM for instance")
 		return err
 	}
 

cri/firecracker/service.go

@@ -35,14 +35,16 @@ import (
 )
 
 const (
-	userContainerName     = "user-container"
-	queueProxyName        = "queue-proxy"
-	revisionEnv           = "K_REVISION"
-	guestIPEnv            = "GUEST_ADDR"
-	guestPortEnv          = "GUEST_PORT"
-	guestImageEnv         = "GUEST_IMAGE"
-	guestMemorySizeMibEnv = "MEM_SIZE_MB"
-	guestvCPUCountEnv     = "VCPU_COUNT"
+	userContainerName       = "user-container"
+	queueProxyName          = "queue-proxy"
+	revisionEnv             = "K_REVISION"
+	guestIPEnv              = "GUEST_ADDR"
+	guestPortEnv            = "GUEST_PORT"
+	guestImageEnv           = "GUEST_IMAGE"
+	guestMemorySizeMibEnv   = "MEM_SIZE_MB"
+	guestvCPUCountEnv       = "VCPU_COUNT"
+	defaultMemSize uint32   = 256
+	defaultvCPUCount uint32 = 1
 )
 
 type FirecrackerService struct {
@@ -69,6 +71,7 @@ func NewFirecrackerService(orch *ctriface.Orchestrator, snapsCapacityMiB int64,
 		return nil, err
 	}
 	fs.stockRuntimeClient = stockRuntimeClient
+
 	fs.coordinator = newFirecrackerCoordinator(orch, snapsCapacityMiB, isSparseSnaps, isFullLocal)
 	fs.vmConfigs = make(map[string]*VMConfig)
 	return fs, nil
@@ -260,7 +263,7 @@ func getMemorySize(config *criapi.ContainerConfig) (uint32, error) {
 		}
 	}
 
-	return uint32(256), nil
+	return defaultMemSize, nil
 }
 
 func getvCPUCount(config *criapi.ContainerConfig) (uint32, error) {
@@ -276,5 +279,5 @@ func getvCPUCount(config *criapi.ContainerConfig) (uint32, error) {
 		}
 	}
 
-	return uint32(1), nil
+	return defaultvCPUCount, nil
 }

ctriface/iface.go

@@ -485,7 +485,12 @@ func (o *Orchestrator) CreateSnapshot(ctx context.Context, vmID string, snap *sn
 }
 
 // LoadSnapshot Loads a snapshot of a VM
-func (o *Orchestrator) LoadSnapshot(ctx context.Context, vmID string, snap *snapshotting.Snapshot, isFullLocal bool) (_ *StartVMResponse, _ *metrics.Metric, retErr error) {
+func (o *Orchestrator) LoadSnapshot(
+	ctx context.Context,
+	vmID string,
+	snap *snapshotting.Snapshot,
+	isFullLocal bool) (_ *StartVMResponse, _ *metrics.Metric, retErr error) {
+
 	var (
 		loadSnapshotMetric   *metrics.Metric = metrics.NewMetric()
 		tStart               time.Time

snapshotting/deduplicated/manager.go

@@ -79,7 +79,7 @@ func (mgr *ImprovedSnapshotManager) AcquireSnapshot(revision string) (*snapshott
 
 	// Snapshot registered in manager but creation not finished yet
 	if ! snapStat.usable { // Could also wait until snapshot usable (trade-off)
-		return nil, errors.New(fmt.Sprintf("Snapshot is not yet usable"))
+		return nil, errors.New("Snapshot is not yet usable")
 	}
 
 	if snapStat.numUsing == 0 {

vhive.go

@@ -56,7 +56,7 @@ var (
 	funcPool *FuncPool
 
 	isSaveMemory       *bool
-	snapsCapacityMiB   *int64
+	snapsStorageSize   *int64
 	isSparseSnaps      *bool
 	isFullLocal        *bool
 	isSnapshotsEnabled *bool
@@ -89,7 +89,7 @@ func main() {
 	isSnapshotsEnabled = flag.Bool("snapshots", false, "Use VM snapshots when adding function instances")
 	isSparseSnaps = flag.Bool("sparsesnaps", false, "Makes memory files sparse after storing to reduce disk utilization")
 	isFullLocal = flag.Bool("fulllocal", false, "Use improved full local snapshotting")
-	snapsCapacityMiB = flag.Int64("snapcapacity", 102400, "Capacity set aside for storing snapshots (Mib)")
+	snapsStorageSize = flag.Int64("snapcapacity", 100, "Total storage reserved for storing snapshots (GiB)")
 	isUPFEnabled = flag.Bool("upf", false, "Enable user-level page faults guest memory management")
 	isLazyMode = flag.Bool("lazy", false, "Enable lazy serving mode when UPFs are enabled")
 
@@ -193,7 +193,8 @@ func setupFirecrackerCRI() {
 
 	s := grpc.NewServer()
 
-	fcService, err := fccri.NewFirecrackerService(orch, *snapsCapacityMiB, *isSparseSnaps, *isFullLocal)
+	snapsCapacityMiB := *snapsStorageSize * 1024
+	fcService, err := fccri.NewFirecrackerService(orch, snapsCapacityMiB, *isSparseSnaps, *isFullLocal)
 	if err != nil {
 		log.Fatalf("failed to create firecracker service %v", err)
 	}