Merge pull request #18820 from mqliang/image-gc-bug

Auto commit by PR queue bot

Godeps/Godeps.json

@@ -831,6 +831,10 @@
 			"ImportPath": "github.com/spf13/pflag",
 			"Rev": "08b1a584251b5b62f458943640fc8ebd4d50aaa5"
 		},
+		{
+			"ImportPath": "github.com/ssoroka/ttime",
+			"Rev": "881f221816e0300201ac24f6c31e54e3bb958de7"
+		},
 		{
 			"ImportPath": "github.com/stretchr/objx",
 			"Rev": "d40df0cc104c06eae2dfe03d7dddb83802d52f9a"

Godeps/_workspace/src/github.com/ssoroka/ttime/README.md

@@ -0,0 +1,42 @@
+This is an experiment in making time easier to mock in Go tests.
+
+You should be able to alias the ttime library to time to avoid having to change all your time.Now() methods to ttime.Now() throughout your code.
+
+All methods return actual time.Time structs (if they were supposed to).
+
+example code:
+
+    import (
+      time "github.com/ssoroka/ttime"
+    )
+
+    fmt.Printf("The starting time is %v", time.Now().UTC())
+
+    // in test this will not sleep at all, but it will advance the clock 5 seconds.
+    // in production, it's identical to time.Sleep
+    time.Sleep(5 * time.Second)
+    fmt.Printf("The time after sleeping for 5 seconds is %v", time.Now().UTC())
+
+    time.After(10 * time.Second, func() {
+      // This will execute after 10 seconds in production and immediately in tests.
+      fmt.Printf("It is now %v", time.Now().UTC())
+    })
+
+example test:
+
+    func TestFreezingTime(t *testing.T) {
+      time.Freeze(time.Now()) // freeze the system clock, at least as far as ttime is concerned.
+
+      // or freeze time at a specific date/time (eg, test leap-year support!):
+      now, err := time.Parse(time.RFC3339, "2012-02-29T00:00:00Z")
+      if err != nil { panic("date time parse failed") }
+      time.Freeze(now)
+      defer time.Unfreeze()
+
+      // test leap-year-specific code
+      if !isLeapYear(time.Now()) {
+        t.Error("Oh no! isLeapYear is broken!")
+      }
+
+      t.Logf("It is now %v", time.Now().UTC())
+    }

Godeps/_workspace/src/github.com/ssoroka/ttime/ttime.go

@@ -0,0 +1,108 @@
+package ttime
+
+import "time"
+
+var (
+  currentTime time.Time
+  timeFrozen  bool
+)
+
+type Duration time.Duration
+type Location time.Location
+type Month time.Month
+type ParseError time.ParseError
+type Ticker time.Ticker
+type Time time.Time
+type Timer time.Timer
+type Weekday time.Weekday
+
+var (
+  // import a ton of constants so we can act like the time library.
+  Parse           = time.Parse
+  ParseDuration   = time.ParseDuration
+  Date            = time.Date
+  ParseInLocation = time.ParseInLocation
+  FixedZone       = time.FixedZone
+  LoadLocation    = time.LoadLocation
+  Sunday          = time.Sunday
+  Monday          = time.Monday
+  Tuesday         = time.Tuesday
+  Wednesday       = time.Wednesday
+  Thursday        = time.Thursday
+  Friday          = time.Friday
+  Saturday        = time.Saturday
+  ANSIC           = time.ANSIC
+  UnixDate        = time.UnixDate
+  RubyDate        = time.RubyDate
+  RFC822          = time.RFC822
+  RFC822Z         = time.RFC822Z
+  RFC850          = time.RFC850
+  RFC1123         = time.RFC1123
+  RFC1123Z        = time.RFC1123Z
+  RFC3339         = time.RFC3339
+  RFC3339Nano     = time.RFC3339Nano
+  Kitchen         = time.Kitchen
+  Stamp           = time.Stamp
+  StampMilli      = time.StampMilli
+  StampMicro      = time.StampMicro
+  StampNano       = time.StampNano
+  // constants that I really should redefine:
+  NewTimer  = time.NewTimer
+  NewTicker = time.NewTicker
+  Unix      = time.Unix
+)
+
+func Freeze(t time.Time) {
+  currentTime = t
+  timeFrozen = true
+}
+
+func Unfreeze() {
+  timeFrozen = false
+}
+
+func IsFrozen() bool {
+  return timeFrozen
+}
+
+func Now() time.Time {
+  if timeFrozen {
+    return currentTime
+  } else {
+    return time.Now()
+  }
+}
+
+func After(d time.Duration) <-chan time.Time {
+  if timeFrozen {
+    currentTime = currentTime.Add(d)
+    c := make(chan time.Time, 1)
+    c <- currentTime
+    return c
+  } else {
+    return time.After(d)
+  }
+}
+
+func Tick(d time.Duration) <-chan time.Time {
+  if timeFrozen {
+    c := make(chan time.Time, 1)
+    go func() {
+      for {
+        currentTime = currentTime.Add(d)
+        c <- currentTime
+      }
+    }()
+    return c
+  } else {
+    return time.Tick(d)
+  }
+}
+
+func Sleep(d time.Duration) {
+  if timeFrozen && d > 0 {
+    currentTime = currentTime.Add(d)
+  } else {
+    time.Sleep(d)
+  }
+}

pkg/kubelet/image_manager.go

@@ -32,6 +32,10 @@ import (
 	"k8s.io/kubernetes/pkg/util/sets"
 )
 
+const (
+	defaultGCAge = time.Minute * 1
+)
+
 // Manages lifecycle of all images.
 //
 // Implementation is thread-safe.
@@ -71,6 +75,10 @@ type realImageManager struct {
 	// The image garbage collection policy in use.
 	policy ImageGCPolicy
 
+	// Minimum age at which a image can be garbage collected, zero for no limit.
+	// TODO(mqliang): move it to ImageGCPolicy and make it configurable
+	minAge time.Duration
+
 	// cAdvisor instance.
 	cadvisor cadvisor.Interface
 
@@ -87,7 +95,7 @@ type realImageManager struct {
 // Information about the images we track.
 type imageRecord struct {
 	// Time when this image was first detected.
-	detected time.Time
+	firstDetected time.Time
 
 	// Time when we last saw this image being used.
 	lastUsed time.Time
@@ -110,6 +118,7 @@ func newImageManager(runtime container.Runtime, cadvisorInterface cadvisor.Inter
 	im := &realImageManager{
 		runtime:      runtime,
 		policy:       policy,
+		minAge:       defaultGCAge,
 		imageRecords: make(map[string]*imageRecord),
 		cadvisor:     cadvisorInterface,
 		recorder:     recorder,
@@ -176,7 +185,7 @@ func (im *realImageManager) detectImages(detected time.Time) error {
 		// New image, set it as detected now.
 		if _, ok := im.imageRecords[image.ID]; !ok {
 			im.imageRecords[image.ID] = &imageRecord{
-				detected: detected,
+				firstDetected: detected,
 			}
 		}
 
@@ -269,6 +278,12 @@ func (im *realImageManager) freeSpace(bytesToFree int64) (int64, error) {
 			break
 		}
 
+		// Avoid garbage collect the image if the image is not old enough.
+		// In such a case, the image may have just been pulled down, and will be used by a container right away.
+		if startTime.Sub(image.firstDetected) < im.minAge {
+			continue
+		}
+
 		// Remove image. Continue despite errors.
 		glog.Infof("[ImageManager]: Removing image %q to free %d bytes", image.id, image.size)
 		err := im.runtime.RemoveImage(container.ImageSpec{Image: image.id})
@@ -299,7 +314,7 @@ func (ev byLastUsedAndDetected) Swap(i, j int) { ev[i], ev[j] = ev[j], ev[i] }
 func (ev byLastUsedAndDetected) Less(i, j int) bool {
 	// Sort by last used, break ties by detected.
 	if ev[i].lastUsed.Equal(ev[j].lastUsed) {
-		return ev[i].detected.Before(ev[j].detected)
+		return ev[i].firstDetected.Before(ev[j].firstDetected)
 	} else {
 		return ev[i].lastUsed.Before(ev[j].lastUsed)
 	}

pkg/kubelet/image_manager_test.go

@@ -22,6 +22,7 @@ import (
 	"time"
 
 	cadvisorapiv2 "github.com/google/cadvisor/info/v2"
+	"github.com/ssoroka/ttime"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"k8s.io/kubernetes/pkg/client/record"
@@ -37,6 +38,7 @@ func newRealImageManager(policy ImageGCPolicy) (*realImageManager, *container.Fa
 	return &realImageManager{
 		runtime:      fakeRuntime,
 		policy:       policy,
+		minAge:       0,
 		imageRecords: make(map[string]*imageRecord),
 		cadvisor:     mockCadvisor,
 		recorder:     &record.FakeRecorder{},
@@ -99,11 +101,11 @@ func TestDetectImagesInitialDetect(t *testing.T) {
 	assert.Equal(manager.imageRecordsLen(), 2)
 	noContainer, ok := manager.getImageRecord(imageName(0))
 	require.True(t, ok)
-	assert.Equal(zero, noContainer.detected)
+	assert.Equal(zero, noContainer.firstDetected)
 	assert.Equal(zero, noContainer.lastUsed)
 	withContainer, ok := manager.getImageRecord(imageName(1))
 	require.True(t, ok)
-	assert.Equal(zero, withContainer.detected)
+	assert.Equal(zero, withContainer.firstDetected)
 	assert.True(withContainer.lastUsed.After(startTime))
 }
 
@@ -141,15 +143,15 @@ func TestDetectImagesWithNewImage(t *testing.T) {
 	assert.Equal(manager.imageRecordsLen(), 3)
 	noContainer, ok := manager.getImageRecord(imageName(0))
 	require.True(t, ok)
-	assert.Equal(zero, noContainer.detected)
+	assert.Equal(zero, noContainer.firstDetected)
 	assert.Equal(zero, noContainer.lastUsed)
 	withContainer, ok := manager.getImageRecord(imageName(1))
 	require.True(t, ok)
-	assert.Equal(zero, withContainer.detected)
+	assert.Equal(zero, withContainer.firstDetected)
 	assert.True(withContainer.lastUsed.After(startTime))
 	newContainer, ok := manager.getImageRecord(imageName(2))
 	require.True(t, ok)
-	assert.Equal(detectedTime, newContainer.detected)
+	assert.Equal(detectedTime, newContainer.firstDetected)
 	assert.Equal(zero, noContainer.lastUsed)
 }
 
@@ -181,11 +183,11 @@ func TestDetectImagesContainerStopped(t *testing.T) {
 	assert.Equal(manager.imageRecordsLen(), 2)
 	container1, ok := manager.getImageRecord(imageName(0))
 	require.True(t, ok)
-	assert.Equal(zero, container1.detected)
+	assert.Equal(zero, container1.firstDetected)
 	assert.Equal(zero, container1.lastUsed)
 	container2, ok := manager.getImageRecord(imageName(1))
 	require.True(t, ok)
-	assert.Equal(zero, container2.detected)
+	assert.Equal(zero, container2.firstDetected)
 	assert.True(container2.lastUsed.Equal(withContainer.lastUsed))
 }
 
@@ -399,3 +401,49 @@ func TestGarbageCollectNotEnoughFreed(t *testing.T) {
 
 	assert.NotNil(t, manager.GarbageCollect())
 }
+
+func TestGarbageCollectImageNotOldEnough(t *testing.T) {
+	policy := ImageGCPolicy{
+		HighThresholdPercent: 90,
+		LowThresholdPercent:  80,
+	}
+	fakeRuntime := &container.FakeRuntime{}
+	mockCadvisor := new(cadvisor.Mock)
+	manager := &realImageManager{
+		runtime:      fakeRuntime,
+		policy:       policy,
+		minAge:       defaultGCAge,
+		imageRecords: make(map[string]*imageRecord),
+		cadvisor:     mockCadvisor,
+		recorder:     &record.FakeRecorder{},
+	}
+
+	fakeRuntime.ImageList = []container.Image{
+		makeImage(0, 1024),
+		makeImage(1, 2048),
+	}
+	// 1 image is in use, and another one is not old enough
+	fakeRuntime.AllPodList = []*container.Pod{
+		{
+			Containers: []*container.Container{
+				makeContainer(1),
+			},
+		},
+	}
+
+	require.NoError(t, manager.detectImages(ttime.Now()))
+	require.Equal(t, manager.imageRecordsLen(), 2)
+	// no space freed since one image is in used, and another one is not old enough
+	spaceFreed, err := manager.freeSpace(1024)
+	assert := assert.New(t)
+	require.NoError(t, err)
+	assert.EqualValues(0, spaceFreed)
+	assert.Len(fakeRuntime.ImageList, 2)
+
+	// sleep 1 minute, then 1 image will be garbage collected
+	ttime.Sleep(defaultGCAge)
+	spaceFreed, err = manager.freeSpace(1024)
+	require.NoError(t, err)
+	assert.EqualValues(1024, spaceFreed)
+	assert.Len(fakeRuntime.ImageList, 1)
+}