Merge pull request #126550 from BnjmnZmmrmn/master

cleanup: avoid unnecessary CRI call when pulling image for pod
This commit is contained in:
Kubernetes Prow Robot 2024-09-11 23:21:11 +01:00 committed by GitHub
commit a568c11bbb
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 212 additions and 66 deletions

View File

@ -73,19 +73,35 @@ func NewImageManager(recorder record.EventRecorder, imageService kubecontainer.I
} }
} }
// shouldPullImage returns whether we should pull an image according to // imagePullPrecheck inspects the pull policy and checks for image presence accordingly,
// the presence and pull policy of the image. // returning (imageRef, error msg, err) and logging any errors.
func shouldPullImage(pullPolicy v1.PullPolicy, imagePresent bool) bool { func (m *imageManager) imagePullPrecheck(ctx context.Context, objRef *v1.ObjectReference, logPrefix string, pullPolicy v1.PullPolicy, spec *kubecontainer.ImageSpec, imgRef string) (imageRef string, msg string, err error) {
if pullPolicy == v1.PullNever { switch pullPolicy {
return false case v1.PullAlways:
return "", msg, nil
case v1.PullIfNotPresent:
imageRef, err = m.imageService.GetImageRef(ctx, *spec)
if err != nil {
msg = fmt.Sprintf("Failed to inspect image %q: %v", imageRef, err)
m.logIt(objRef, v1.EventTypeWarning, events.FailedToInspectImage, logPrefix, msg, klog.Warning)
return "", msg, ErrImageInspect
} }
return imageRef, msg, nil
if pullPolicy == v1.PullAlways || case v1.PullNever:
(pullPolicy == v1.PullIfNotPresent && (!imagePresent)) { imageRef, err = m.imageService.GetImageRef(ctx, *spec)
return true if err != nil {
msg = fmt.Sprintf("Failed to inspect image %q: %v", imageRef, err)
m.logIt(objRef, v1.EventTypeWarning, events.FailedToInspectImage, logPrefix, msg, klog.Warning)
return "", msg, ErrImageInspect
} }
if imageRef == "" {
return false msg = fmt.Sprintf("Container image %q is not present with pull policy of Never", imgRef)
m.logIt(objRef, v1.EventTypeWarning, events.ErrImageNeverPullPolicy, logPrefix, msg, klog.Warning)
return "", msg, ErrImageNeverPull
}
return imageRef, msg, nil
}
return
} }
// records an event using ref, event msg. log to glog using prefix, msg, logFn // records an event using ref, event msg. log to glog using prefix, msg, logFn
@ -124,23 +140,14 @@ func (m *imageManager) EnsureImageExists(ctx context.Context, objRef *v1.ObjectR
RuntimeHandler: podRuntimeHandler, RuntimeHandler: podRuntimeHandler,
} }
imageRef, err = m.imageService.GetImageRef(ctx, spec) imageRef, message, err = m.imagePullPrecheck(ctx, objRef, logPrefix, pullPolicy, &spec, imgRef)
if err != nil { if err != nil {
msg := fmt.Sprintf("Failed to inspect image %q: %v", imgRef, err) return "", message, err
m.logIt(objRef, v1.EventTypeWarning, events.FailedToInspectImage, logPrefix, msg, klog.Warning)
return "", msg, ErrImageInspect
} }
if imageRef != "" {
present := imageRef != ""
if !shouldPullImage(pullPolicy, present) {
if present {
msg := fmt.Sprintf("Container image %q already present on machine", imgRef) msg := fmt.Sprintf("Container image %q already present on machine", imgRef)
m.logIt(objRef, v1.EventTypeNormal, events.PulledImage, logPrefix, msg, klog.Info) m.logIt(objRef, v1.EventTypeNormal, events.PulledImage, logPrefix, msg, klog.Info)
return imageRef, "", nil return imageRef, msg, nil
}
msg := fmt.Sprintf("Container image %q is not present with pull policy of Never", imgRef)
m.logIt(objRef, v1.EventTypeWarning, events.ErrImageNeverPullPolicy, logPrefix, msg, klog.Warning)
return "", msg, ErrImageNeverPull
} }
backOffKey := fmt.Sprintf("%s_%s", pod.UID, imgRef) backOffKey := fmt.Sprintf("%s_%s", pod.UID, imgRef)

View File

@ -24,20 +24,23 @@ import (
"testing" "testing"
"time" "time"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
v1 "k8s.io/api/core/v1" v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/types"
utilfeature "k8s.io/apiserver/pkg/util/feature" utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/client-go/tools/record"
"k8s.io/client-go/util/flowcontrol" "k8s.io/client-go/util/flowcontrol"
featuregatetesting "k8s.io/component-base/featuregate/testing" featuregatetesting "k8s.io/component-base/featuregate/testing"
crierrors "k8s.io/cri-api/pkg/errors" crierrors "k8s.io/cri-api/pkg/errors"
"k8s.io/kubernetes/pkg/controller/testutil"
"k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/features"
. "k8s.io/kubernetes/pkg/kubelet/container" . "k8s.io/kubernetes/pkg/kubelet/container"
ctest "k8s.io/kubernetes/pkg/kubelet/container/testing" ctest "k8s.io/kubernetes/pkg/kubelet/container/testing"
"k8s.io/kubernetes/test/utils/ktesting"
testingclock "k8s.io/utils/clock/testing" testingclock "k8s.io/utils/clock/testing"
utilpointer "k8s.io/utils/pointer" "k8s.io/utils/ptr"
) )
type pullerExpects struct { type pullerExpects struct {
@ -45,6 +48,7 @@ type pullerExpects struct {
err error err error
shouldRecordStartedPullingTime bool shouldRecordStartedPullingTime bool
shouldRecordFinishedPullingTime bool shouldRecordFinishedPullingTime bool
events []v1.Event
} }
type pullerTestCase struct { type pullerTestCase struct {
@ -69,7 +73,11 @@ func pullerTestCases() []pullerTestCase {
qps: 0.0, qps: 0.0,
burst: 0, burst: 0,
expected: []pullerExpects{ expected: []pullerExpects{
{[]string{"GetImageRef", "PullImage", "GetImageSize"}, nil, true, true}, {[]string{"GetImageRef", "PullImage", "GetImageSize"}, nil, true, true,
[]v1.Event{
{Reason: "Pulling"},
{Reason: "Pulled"},
}},
}}, }},
{ // image present, don't pull { // image present, don't pull
@ -81,9 +89,18 @@ func pullerTestCases() []pullerTestCase {
qps: 0.0, qps: 0.0,
burst: 0, burst: 0,
expected: []pullerExpects{ expected: []pullerExpects{
{[]string{"GetImageRef"}, nil, false, false}, {[]string{"GetImageRef"}, nil, false, false,
{[]string{"GetImageRef"}, nil, false, false}, []v1.Event{
{[]string{"GetImageRef"}, nil, false, false}, {Reason: "Pulled"},
}},
{[]string{"GetImageRef"}, nil, false, false,
[]v1.Event{
{Reason: "Pulled"},
}},
{[]string{"GetImageRef"}, nil, false, false,
[]v1.Event{
{Reason: "Pulled"},
}},
}}, }},
// image present, pull it // image present, pull it
{containerImage: "present_image", {containerImage: "present_image",
@ -94,9 +111,21 @@ func pullerTestCases() []pullerTestCase {
qps: 0.0, qps: 0.0,
burst: 0, burst: 0,
expected: []pullerExpects{ expected: []pullerExpects{
{[]string{"GetImageRef", "PullImage", "GetImageSize"}, nil, true, true}, {[]string{"PullImage", "GetImageSize"}, nil, true, true,
{[]string{"GetImageRef", "PullImage", "GetImageSize"}, nil, true, true}, []v1.Event{
{[]string{"GetImageRef", "PullImage", "GetImageSize"}, nil, true, true}, {Reason: "Pulling"},
{Reason: "Pulled"},
}},
{[]string{"PullImage", "GetImageSize"}, nil, true, true,
[]v1.Event{
{Reason: "Pulling"},
{Reason: "Pulled"},
}},
{[]string{"PullImage", "GetImageSize"}, nil, true, true,
[]v1.Event{
{Reason: "Pulling"},
{Reason: "Pulled"},
}},
}}, }},
// missing image, error PullNever // missing image, error PullNever
{containerImage: "missing_image", {containerImage: "missing_image",
@ -107,9 +136,18 @@ func pullerTestCases() []pullerTestCase {
qps: 0.0, qps: 0.0,
burst: 0, burst: 0,
expected: []pullerExpects{ expected: []pullerExpects{
{[]string{"GetImageRef"}, ErrImageNeverPull, false, false}, {[]string{"GetImageRef"}, ErrImageNeverPull, false, false,
{[]string{"GetImageRef"}, ErrImageNeverPull, false, false}, []v1.Event{
{[]string{"GetImageRef"}, ErrImageNeverPull, false, false}, {Reason: "ErrImageNeverPull"},
}},
{[]string{"GetImageRef"}, ErrImageNeverPull, false, false,
[]v1.Event{
{Reason: "ErrImageNeverPull"},
}},
{[]string{"GetImageRef"}, ErrImageNeverPull, false, false,
[]v1.Event{
{Reason: "ErrImageNeverPull"},
}},
}}, }},
// missing image, unable to inspect // missing image, unable to inspect
{containerImage: "missing_image", {containerImage: "missing_image",
@ -120,9 +158,18 @@ func pullerTestCases() []pullerTestCase {
qps: 0.0, qps: 0.0,
burst: 0, burst: 0,
expected: []pullerExpects{ expected: []pullerExpects{
{[]string{"GetImageRef"}, ErrImageInspect, false, false}, {[]string{"GetImageRef"}, ErrImageInspect, false, false,
{[]string{"GetImageRef"}, ErrImageInspect, false, false}, []v1.Event{
{[]string{"GetImageRef"}, ErrImageInspect, false, false}, {Reason: "InspectFailed"},
}},
{[]string{"GetImageRef"}, ErrImageInspect, false, false,
[]v1.Event{
{Reason: "InspectFailed"},
}},
{[]string{"GetImageRef"}, ErrImageInspect, false, false,
[]v1.Event{
{Reason: "InspectFailed"},
}},
}}, }},
// missing image, unable to fetch // missing image, unable to fetch
{containerImage: "typo_image", {containerImage: "typo_image",
@ -133,12 +180,33 @@ func pullerTestCases() []pullerTestCase {
qps: 0.0, qps: 0.0,
burst: 0, burst: 0,
expected: []pullerExpects{ expected: []pullerExpects{
{[]string{"GetImageRef", "PullImage"}, ErrImagePull, true, false}, {[]string{"GetImageRef", "PullImage"}, ErrImagePull, true, false,
{[]string{"GetImageRef", "PullImage"}, ErrImagePull, true, false}, []v1.Event{
{[]string{"GetImageRef"}, ErrImagePullBackOff, false, false}, {Reason: "Pulling"},
{[]string{"GetImageRef", "PullImage"}, ErrImagePull, true, false}, {Reason: "Failed"},
{[]string{"GetImageRef"}, ErrImagePullBackOff, false, false}, }},
{[]string{"GetImageRef"}, ErrImagePullBackOff, false, false}, {[]string{"GetImageRef", "PullImage"}, ErrImagePull, true, false,
[]v1.Event{
{Reason: "Pulling"},
{Reason: "Failed"},
}},
{[]string{"GetImageRef"}, ErrImagePullBackOff, false, false,
[]v1.Event{
{Reason: "BackOff"},
}},
{[]string{"GetImageRef", "PullImage"}, ErrImagePull, true, false,
[]v1.Event{
{Reason: "Pulling"},
{Reason: "Failed"},
}},
{[]string{"GetImageRef"}, ErrImagePullBackOff, false, false,
[]v1.Event{
{Reason: "BackOff"},
}},
{[]string{"GetImageRef"}, ErrImagePullBackOff, false, false,
[]v1.Event{
{Reason: "BackOff"},
}},
}}, }},
// image present, non-zero qps, try to pull // image present, non-zero qps, try to pull
{containerImage: "present_image", {containerImage: "present_image",
@ -149,9 +217,21 @@ func pullerTestCases() []pullerTestCase {
qps: 400.0, qps: 400.0,
burst: 600, burst: 600,
expected: []pullerExpects{ expected: []pullerExpects{
{[]string{"GetImageRef", "PullImage", "GetImageSize"}, nil, true, true}, {[]string{"PullImage", "GetImageSize"}, nil, true, true,
{[]string{"GetImageRef", "PullImage", "GetImageSize"}, nil, true, true}, []v1.Event{
{[]string{"GetImageRef", "PullImage", "GetImageSize"}, nil, true, true}, {Reason: "Pulling"},
{Reason: "Pulled"},
}},
{[]string{"PullImage", "GetImageSize"}, nil, true, true,
[]v1.Event{
{Reason: "Pulling"},
{Reason: "Pulled"},
}},
{[]string{"PullImage", "GetImageSize"}, nil, true, true,
[]v1.Event{
{Reason: "Pulling"},
{Reason: "Pulled"},
}},
}}, }},
// image present, non-zero qps, try to pull when qps exceeded // image present, non-zero qps, try to pull when qps exceeded
{containerImage: "present_image", {containerImage: "present_image",
@ -162,9 +242,20 @@ func pullerTestCases() []pullerTestCase {
qps: 2000.0, qps: 2000.0,
burst: 0, burst: 0,
expected: []pullerExpects{ expected: []pullerExpects{
{[]string{"GetImageRef"}, ErrImagePull, true, false}, {[]string(nil), ErrImagePull, true, false,
{[]string{"GetImageRef"}, ErrImagePull, true, false}, []v1.Event{
{[]string{"GetImageRef"}, ErrImagePullBackOff, false, false}, {Reason: "Pulling"},
{Reason: "Failed"},
}},
{[]string(nil), ErrImagePull, true, false,
[]v1.Event{
{Reason: "Pulling"},
{Reason: "Failed"},
}},
{[]string(nil), ErrImagePullBackOff, false, false,
[]v1.Event{
{Reason: "BackOff"},
}},
}}, }},
// error case if image name fails validation due to invalid reference format // error case if image name fails validation due to invalid reference format
{containerImage: "FAILED_IMAGE", {containerImage: "FAILED_IMAGE",
@ -175,7 +266,10 @@ func pullerTestCases() []pullerTestCase {
qps: 0.0, qps: 0.0,
burst: 0, burst: 0,
expected: []pullerExpects{ expected: []pullerExpects{
{[]string(nil), ErrInvalidImageName, false, false}, {[]string(nil), ErrInvalidImageName, false, false,
[]v1.Event{
{Reason: "InspectFailed"},
}},
}}, }},
// error case if image name contains http // error case if image name contains http
{containerImage: "http://url", {containerImage: "http://url",
@ -186,7 +280,10 @@ func pullerTestCases() []pullerTestCase {
qps: 0.0, qps: 0.0,
burst: 0, burst: 0,
expected: []pullerExpects{ expected: []pullerExpects{
{[]string(nil), ErrInvalidImageName, false, false}, {[]string(nil), ErrInvalidImageName, false, false,
[]v1.Event{
{Reason: "InspectFailed"},
}},
}}, }},
// error case if image name contains sha256 // error case if image name contains sha256
{containerImage: "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad", {containerImage: "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad",
@ -197,7 +294,10 @@ func pullerTestCases() []pullerTestCase {
qps: 0.0, qps: 0.0,
burst: 0, burst: 0,
expected: []pullerExpects{ expected: []pullerExpects{
{[]string(nil), ErrInvalidImageName, false, false}, {[]string(nil), ErrInvalidImageName, false, false,
[]v1.Event{
{Reason: "InspectFailed"},
}},
}}, }},
} }
} }
@ -227,7 +327,7 @@ func (m *mockPodPullingTimeRecorder) reset() {
m.finishedPullingRecorded = false m.finishedPullingRecorded = false
} }
func pullerTestEnv(t *testing.T, c pullerTestCase, serialized bool, maxParallelImagePulls *int32) (puller ImageManager, fakeClock *testingclock.FakeClock, fakeRuntime *ctest.FakeRuntime, container *v1.Container, fakePodPullingTimeRecorder *mockPodPullingTimeRecorder) { func pullerTestEnv(t *testing.T, c pullerTestCase, serialized bool, maxParallelImagePulls *int32) (puller ImageManager, fakeClock *testingclock.FakeClock, fakeRuntime *ctest.FakeRuntime, container *v1.Container, fakePodPullingTimeRecorder *mockPodPullingTimeRecorder, fakeRecorder *testutil.FakeRecorder) {
container = &v1.Container{ container = &v1.Container{
Name: "container_name", Name: "container_name",
Image: c.containerImage, Image: c.containerImage,
@ -239,7 +339,7 @@ func pullerTestEnv(t *testing.T, c pullerTestCase, serialized bool, maxParallelI
backOff.Clock = fakeClock backOff.Clock = fakeClock
fakeRuntime = &ctest.FakeRuntime{T: t} fakeRuntime = &ctest.FakeRuntime{T: t}
fakeRecorder := &record.FakeRecorder{} fakeRecorder = testutil.NewFakeRecorder()
fakeRuntime.ImageList = []Image{{ID: "present_image:latest"}} fakeRuntime.ImageList = []Image{{ID: "present_image:latest"}}
fakeRuntime.Err = c.pullerErr fakeRuntime.Err = c.pullerErr
@ -266,7 +366,7 @@ func TestParallelPuller(t *testing.T) {
for _, c := range cases { for _, c := range cases {
t.Run(c.testName, func(t *testing.T) { t.Run(c.testName, func(t *testing.T) {
ctx := context.Background() ctx := context.Background()
puller, fakeClock, fakeRuntime, container, fakePodPullingTimeRecorder := pullerTestEnv(t, c, useSerializedEnv, nil) puller, fakeClock, fakeRuntime, container, fakePodPullingTimeRecorder, _ := pullerTestEnv(t, c, useSerializedEnv, nil)
for _, expected := range c.expected { for _, expected := range c.expected {
fakeRuntime.CalledFunctions = nil fakeRuntime.CalledFunctions = nil
@ -298,7 +398,7 @@ func TestSerializedPuller(t *testing.T) {
for _, c := range cases { for _, c := range cases {
t.Run(c.testName, func(t *testing.T) { t.Run(c.testName, func(t *testing.T) {
ctx := context.Background() ctx := context.Background()
puller, fakeClock, fakeRuntime, container, fakePodPullingTimeRecorder := pullerTestEnv(t, c, useSerializedEnv, nil) puller, fakeClock, fakeRuntime, container, fakePodPullingTimeRecorder, _ := pullerTestEnv(t, c, useSerializedEnv, nil)
for _, expected := range c.expected { for _, expected := range c.expected {
fakeRuntime.CalledFunctions = nil fakeRuntime.CalledFunctions = nil
@ -356,13 +456,13 @@ func TestPullAndListImageWithPodAnnotations(t *testing.T) {
inspectErr: nil, inspectErr: nil,
pullerErr: nil, pullerErr: nil,
expected: []pullerExpects{ expected: []pullerExpects{
{[]string{"GetImageRef", "PullImage", "GetImageSize"}, nil, true, true}, {[]string{"GetImageRef", "PullImage", "GetImageSize"}, nil, true, true, nil},
}} }}
useSerializedEnv := true useSerializedEnv := true
t.Run(c.testName, func(t *testing.T) { t.Run(c.testName, func(t *testing.T) {
ctx := context.Background() ctx := context.Background()
puller, fakeClock, fakeRuntime, container, fakePodPullingTimeRecorder := pullerTestEnv(t, c, useSerializedEnv, nil) puller, fakeClock, fakeRuntime, container, fakePodPullingTimeRecorder, _ := pullerTestEnv(t, c, useSerializedEnv, nil)
fakeRuntime.CalledFunctions = nil fakeRuntime.CalledFunctions = nil
fakeRuntime.ImageList = []Image{} fakeRuntime.ImageList = []Image{}
fakeClock.Step(time.Second) fakeClock.Step(time.Second)
@ -412,14 +512,14 @@ func TestPullAndListImageWithRuntimeHandlerInImageCriAPIFeatureGate(t *testing.T
inspectErr: nil, inspectErr: nil,
pullerErr: nil, pullerErr: nil,
expected: []pullerExpects{ expected: []pullerExpects{
{[]string{"GetImageRef", "PullImage", "GetImageSize"}, nil, true, true}, {[]string{"GetImageRef", "PullImage", "GetImageSize"}, nil, true, true, nil},
}} }}
useSerializedEnv := true useSerializedEnv := true
t.Run(c.testName, func(t *testing.T) { t.Run(c.testName, func(t *testing.T) {
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RuntimeClassInImageCriAPI, true) featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RuntimeClassInImageCriAPI, true)
ctx := context.Background() ctx := context.Background()
puller, fakeClock, fakeRuntime, container, fakePodPullingTimeRecorder := pullerTestEnv(t, c, useSerializedEnv, nil) puller, fakeClock, fakeRuntime, container, fakePodPullingTimeRecorder, _ := pullerTestEnv(t, c, useSerializedEnv, nil)
fakeRuntime.CalledFunctions = nil fakeRuntime.CalledFunctions = nil
fakeRuntime.ImageList = []Image{} fakeRuntime.ImageList = []Image{}
fakeClock.Step(time.Second) fakeClock.Step(time.Second)
@ -473,7 +573,7 @@ func TestMaxParallelImagePullsLimit(t *testing.T) {
maxParallelImagePulls := 5 maxParallelImagePulls := 5
var wg sync.WaitGroup var wg sync.WaitGroup
puller, fakeClock, fakeRuntime, container, _ := pullerTestEnv(t, *testCase, useSerializedEnv, utilpointer.Int32Ptr(int32(maxParallelImagePulls))) puller, fakeClock, fakeRuntime, container, _, _ := pullerTestEnv(t, *testCase, useSerializedEnv, ptr.To(int32(maxParallelImagePulls)))
fakeRuntime.BlockImagePulls = true fakeRuntime.BlockImagePulls = true
fakeRuntime.CalledFunctions = nil fakeRuntime.CalledFunctions = nil
fakeRuntime.T = t fakeRuntime.T = t
@ -573,3 +673,42 @@ func TestEvalCRIPullErr(t *testing.T) {
}) })
} }
} }
func TestImagePullPrecheck(t *testing.T) {
pod := &v1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "test_pod",
Namespace: "test-ns",
UID: "bar",
ResourceVersion: "42",
}}
cases := pullerTestCases()
useSerializedEnv := true
for _, c := range cases {
t.Run(c.testName, func(t *testing.T) {
ctx := ktesting.Init(t)
puller, fakeClock, fakeRuntime, container, _, fakeRecorder := pullerTestEnv(t, c, useSerializedEnv, nil)
for _, expected := range c.expected {
fakeRuntime.CalledFunctions = nil
fakeRecorder.Events = []*v1.Event{}
fakeClock.Step(time.Second)
_, _, err := puller.EnsureImageExists(ctx, &v1.ObjectReference{}, pod, container.Image, nil, nil, "", container.ImagePullPolicy)
fakeRuntime.AssertCalls(expected.calls)
var recorderEvents []v1.Event
for _, event := range fakeRecorder.Events {
recorderEvents = append(recorderEvents, v1.Event{Reason: event.Reason})
}
if diff := cmp.Diff(recorderEvents, expected.events); diff != "" {
t.Errorf("unexpected events diff (-want +got):\n%s", diff)
}
if diff := cmp.Diff(expected.err, err, cmpopts.EquateErrors()); diff != "" {
ctx.Errorf("did not get expected error: %v\ndiff (-want, +got):\n%s", err, diff)
}
}
})
}
}