Updating image puller package with the new pod flow

This commit contains the following changes:
1. Removing image label - image will be obtained from the Spec directly
2. Adding pod type label - one-time pod that should be considered as
   Failed once the container fails on image pull, and run-until-success, that is considered
   as InProgess, in case container fails on image pull and there are build instructions
3. Adding GetPodStatus API - this api returns pod status based on the
   following:
   - the pod that fails to pull image stays in the Pending state, and
     the container status is updated with appropriate reason.So, if the
pod is one-time pod, and container failed on image pull - status is
Failed. Otherwise status is Running, since we let the pod restart the
container with back-offs
4. Pod is created with restartPolicy Never, since if the pod managed to
   pull image - we consider it a success

Author: yevgeny-shnaidman

internal/pod/imagepuller.go

@@ -12,6 +12,26 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
+type PullPodStatus string
+
+const (
+	PullImageFailed        PullPodStatus = "pullFailed"
+	PullImageSuccess       PullPodStatus = "pullSuccess"
+	PullImageInProcess     PullPodStatus = "pullInProcess"
+	PullImageUnexpectedErr PullPodStatus = "unexpectedError"
+
+	imagePullBackOffReason = "ImagePullBackOff"
+	errImagePullReason     = "ErrImagePull"
+
+	moduleImageLabelKey = "kmm.node.kubernetes.io/module-image-config"
+	pullPodTypeLabelKey = "kmm.node.kubernetes.io/pull-pod-type"
+
+	pullerContainerName = "puller"
+
+	pullPodTypeOneTime  = "one-time-pull"
+	pullPodUntilSuccess = "until-success"
+)
+
 //go:generate mockgen -source=imagepuller.go -package=pod -destination=mock_imagepuller.go
 
 type ImagePuller interface {
@@ -20,14 +40,9 @@ type ImagePuller interface {
 	ListPullPods(ctx context.Context, micObj *kmmv1beta1.ModuleImagesConfig) ([]v1.Pod, error)
 	GetPullPodForImage(pods []v1.Pod, image string) *v1.Pod
 	GetPullPodImage(pod v1.Pod) string
+	GetPullPodStatus(pod *v1.Pod) PullPodStatus
 }
 
-const (
-	moduleImageLabelKey = "kmm.node.kubernetes.io/module-image-config"
-	imageLabelKey       = "kmm.node.kubernetes.io/module-image"
-	pullerContainerName = "puller"
-)
-
 type imagePullerImpl struct {
 	client client.Client
 	scheme *runtime.Scheme
@@ -43,9 +58,9 @@ func NewImagePuller(client client.Client, scheme *runtime.Scheme) ImagePuller {
 func (ipi *imagePullerImpl) CreatePullPod(ctx context.Context, imageSpec *kmmv1beta1.ModuleImageSpec,
 	micObj *kmmv1beta1.ModuleImagesConfig) error {
 
-	restartPolicy := v1.RestartPolicyOnFailure
+	pullPodTypeLabeValue := pullPodUntilSuccess
 	if imageSpec.Build != nil || imageSpec.Sign != nil {
-		restartPolicy = v1.RestartPolicyNever
+		pullPodTypeLabeValue = pullPodTypeOneTime
 	}
 
 	imagePullSecrets := []v1.LocalObjectReference{}
@@ -59,7 +74,7 @@ func (ipi *imagePullerImpl) CreatePullPod(ctx context.Context, imageSpec *kmmv1b
 			Namespace:    micObj.Namespace,
 			Labels: map[string]string{
 				moduleImageLabelKey: micObj.Name,
-				imageLabelKey:       imageSpec.Image,
+				pullPodTypeLabelKey: pullPodTypeLabeValue,
 			},
 		},
 		Spec: v1.PodSpec{
@@ -70,7 +85,7 @@ func (ipi *imagePullerImpl) CreatePullPod(ctx context.Context, imageSpec *kmmv1b
 					Command: []string{"/bin/sh", "-c", "exit 0"},
 				},
 			},
-			RestartPolicy:    restartPolicy,
+			RestartPolicy:    v1.RestartPolicyNever,
 			ImagePullSecrets: imagePullSecrets,
 		},
 	}
@@ -92,7 +107,7 @@ func (ipi *imagePullerImpl) ListPullPods(ctx context.Context, micObj *kmmv1beta1
 
 	pl := v1.PodList{}
 
-	hl := client.HasLabels{imageLabelKey}
+	hl := client.HasLabels{pullPodTypeLabelKey}
 	ml := client.MatchingLabels{moduleImageLabelKey: micObj.Name}
 
 	ctrl.LoggerFrom(ctx).WithValues("mic name", micObj.Name).V(1).Info("Listing mic image Pods")
@@ -107,13 +122,46 @@ func (ipi *imagePullerImpl) ListPullPods(ctx context.Context, micObj *kmmv1beta1
 func (ipi *imagePullerImpl) GetPullPodForImage(pods []v1.Pod, image string) *v1.Pod {
 
 	for i, pod := range pods {
-		if image == pod.Labels[imageLabelKey] {
+		if image == pod.Spec.Containers[0].Image {
 			return &pods[i]
 		}
 	}
 	return nil
 }
 
 func (ipi *imagePullerImpl) GetPullPodImage(pod v1.Pod) string {
-	return pod.Labels[imageLabelKey]
+	return pod.Spec.Containers[0].Image
+}
+
+func (ipi *imagePullerImpl) GetPullPodStatus(pod *v1.Pod) PullPodStatus {
+	switch pod.Status.Phase {
+	case v1.PodSucceeded:
+		return PullImageSuccess
+	case v1.PodFailed, v1.PodUnknown:
+		return PullImageUnexpectedErr
+	case v1.PodRunning:
+		return PullImageInProcess
+	case v1.PodPending:
+		// no container statuses yet, the pod is just starting to pull images
+		if pod.Status.ContainerStatuses == nil {
+			return PullImageInProcess
+		}
+
+		// no wating status, the pull process is still in progress
+		if pod.Status.ContainerStatuses[0].State.Waiting == nil {
+			return PullImageInProcess
+		}
+
+		pullPodType := pod.GetLabels()[pullPodTypeLabelKey]
+		// if pod is targeted to wait till the end - return the InProgress
+		if pullPodType == pullPodUntilSuccess {
+			return PullImageInProcess
+		}
+
+		if waitingReason := pod.Status.ContainerStatuses[0].State.Waiting.Reason; waitingReason == imagePullBackOffReason || waitingReason == errImagePullReason {
+			return PullImageFailed
+		}
+	}
+
+	return PullImageUnexpectedErr
 }

internal/pod/imagepuller_test.go

@@ -38,7 +38,7 @@ var _ = Describe("ListPullPods", func() {
 	}
 
 	It("list succeeded", func() {
-		hl := ctrlclient.HasLabels{imageLabelKey}
+		hl := ctrlclient.HasLabels{pullPodTypeLabelKey}
 		ml := ctrlclient.MatchingLabels{moduleImageLabelKey: testMic.Name}
 
 		clnt.EXPECT().List(context.Background(), gomock.Any(), ctrlclient.InNamespace(testMic.Namespace), hl, ml).DoAndReturn(
@@ -54,7 +54,7 @@ var _ = Describe("ListPullPods", func() {
 	})
 
 	It("list failed", func() {
-		hl := ctrlclient.HasLabels{imageLabelKey}
+		hl := ctrlclient.HasLabels{pullPodTypeLabelKey}
 		ml := ctrlclient.MatchingLabels{moduleImageLabelKey: testMic.Name}
 
 		clnt.EXPECT().List(context.Background(), gomock.Any(), ctrlclient.InNamespace(testMic.Namespace), hl, ml).Return(fmt.Errorf("some error"))
@@ -116,21 +116,29 @@ var _ = Describe("GetPullPodForImage", func() {
 
 	pullPods := []v1.Pod{
 		{
-			ObjectMeta: metav1.ObjectMeta{
-				Labels: map[string]string{imageLabelKey: "image 1"},
+			Spec: v1.PodSpec{
+				Containers: []v1.Container{
+					{
+						Image: "image 1",
+					},
+				},
 			},
 		},
 		{
-			ObjectMeta: metav1.ObjectMeta{
-				Labels: map[string]string{imageLabelKey: "image 2"},
+			Spec: v1.PodSpec{
+				Containers: []v1.Container{
+					{
+						Image: "image 2",
+					},
+				},
 			},
 		},
 	}
 
 	It("there is a pull pod for that image", func() {
 		res := ip.GetPullPodForImage(pullPods, "image 2")
 		Expect(res).ToNot(BeNil())
-		Expect(res.Labels[imageLabelKey]).To(Equal("image 2"))
+		Expect(res.Spec.Containers[0].Image).To(Equal("image 2"))
 	})
 
 	It("there is no pull pod for that image", func() {
@@ -170,7 +178,7 @@ var _ = Describe("CreatePullPod", func() {
 				Namespace:    testMic.Namespace,
 				Labels: map[string]string{
 					moduleImageLabelKey: "some name",
-					imageLabelKey:       "some image",
+					pullPodTypeLabelKey: pullPodUntilSuccess,
 				},
 			},
 			Spec: v1.PodSpec{
@@ -181,7 +189,7 @@ var _ = Describe("CreatePullPod", func() {
 						Command: []string{"/bin/sh", "-c", "exit 0"},
 					},
 				},
-				RestartPolicy:    v1.RestartPolicyOnFailure,
+				RestartPolicy:    v1.RestartPolicyNever,
 				ImagePullSecrets: []v1.LocalObjectReference{},
 			},
 		}
@@ -202,3 +210,82 @@ var _ = Describe("CreatePullPod", func() {
 		Expect(err).To(BeNil())
 	})
 })
+
+var _ = Describe("GetPullPodStatus", func() {
+	var (
+		ctrl *gomock.Controller
+		clnt *client.MockClient
+		ip   ImagePuller
+	)
+
+	BeforeEach(func() {
+		ctrl = gomock.NewController(GinkgoT())
+		clnt = client.NewMockClient(ctrl)
+		ip = NewImagePuller(clnt, scheme)
+	})
+
+	testPod := v1.Pod{
+		Status: v1.PodStatus{
+			Phase: v1.PodSucceeded,
+		},
+	}
+
+	It("check different phases except for PodPending", func() {
+		By("phase Succeeded")
+
+		res := ip.GetPullPodStatus(&testPod)
+		Expect(res).To(Equal(PullImageSuccess))
+
+		By("phase PodFailed")
+		testPod.Status.Phase = v1.PodFailed
+		res = ip.GetPullPodStatus(&testPod)
+		Expect(res).To(Equal(PullImageUnexpectedErr))
+
+		By("phase PodUnknown")
+		testPod.Status.Phase = v1.PodUnknown
+		res = ip.GetPullPodStatus(&testPod)
+		Expect(res).To(Equal(PullImageUnexpectedErr))
+
+		By("phase PodRunning")
+		testPod.Status.Phase = v1.PodRunning
+		res = ip.GetPullPodStatus(&testPod)
+		Expect(res).To(Equal(PullImageInProcess))
+	})
+
+	It("check container statuses for PodPending", func() {
+		testPod.Status.Phase = v1.PodPending
+		By("container statuses missing")
+		testPod.Status.ContainerStatuses = nil
+		res := ip.GetPullPodStatus(&testPod)
+		Expect(res).To(Equal(PullImageInProcess))
+
+		By("container statuses waiting is nil")
+		testPod.Status.ContainerStatuses = []v1.ContainerStatus{v1.ContainerStatus{}}
+		res = ip.GetPullPodStatus(&testPod)
+		Expect(res).To(Equal(PullImageInProcess))
+
+		By("container statuses waiting is not nil and pod is not one time pull pod")
+		testPod.SetLabels(map[string]string{pullPodTypeLabelKey: pullPodUntilSuccess})
+		testPod.Status.ContainerStatuses[0].State = v1.ContainerState{
+			Waiting: &v1.ContainerStateWaiting{},
+		}
+		res = ip.GetPullPodStatus(&testPod)
+		Expect(res).To(Equal(PullImageInProcess))
+
+		By("container statuses waiting is not nil and pod is one time pull pod and reason is not image pull error")
+		testPod.SetLabels(map[string]string{pullPodTypeLabelKey: pullPodTypeOneTime})
+		testPod.Status.ContainerStatuses[0].State.Waiting.Reason = "some reason"
+		res = ip.GetPullPodStatus(&testPod)
+		Expect(res).To(Equal(PullImageUnexpectedErr))
+
+		By("container statuses waiting is not nil and pod is one time pull pod and reason is ImagePullBackOff")
+		testPod.Status.ContainerStatuses[0].State.Waiting.Reason = imagePullBackOffReason
+		res = ip.GetPullPodStatus(&testPod)
+		Expect(res).To(Equal(PullImageFailed))
+
+		By("container statuses waiting is not nil and pod is one time pull pod and reason is errImagePullReason")
+		testPod.Status.ContainerStatuses[0].State.Waiting.Reason = errImagePullReason
+		res = ip.GetPullPodStatus(&testPod)
+		Expect(res).To(Equal(PullImageFailed))
+	})
+})