fix: prevent adding failed images to workqueue repeatedly

Signed-off-by: zeroalphat <taichi-takemura@cybozu.co.jp>

Author: zeroalphat

api/v1/nodeimageset_types.go

@@ -84,10 +84,11 @@ type ContainerImageStatus struct {
 }
 
 const (
-	WaitingForImageDownload = "WaitingForImageDownload"
-	ImageDownloaded         = "ImageDownloaded"
-	ImageDownloadInProgress = "ImageDownloadInProgress"
-	ImageDownloadFailed     = "ImageDownloadFailed"
+	WaitingForImageDownload        = "WaitingForImageDownload"
+	ImageDownloaded                = "ImageDownloaded"
+	ImageDownloadInProgress        = "ImageDownloadInProgress"
+	ImageDownloadFailed            = "ImageDownloadFailed"
+	ImageDownloadTemporarilyFailed = "ImageDownloadTemporarilyFailed"
 )
 
 const (

internal/controller/nodeimageset_controller.go

@@ -142,12 +142,12 @@ func (r *NodeImageSetReconciler) collectPendingImages(ctx context.Context, nodeI
 
 	var images []string
 	for _, image := range nodeImageSet.Spec.Images {
-		status, _, err := r.ImagePuller.GetImageStatus(ctx, nodeImageSet.Name, image)
+		status, _, err := r.ImagePuller.GetImageStatus(ctx, nodeImageSet.Name, image, nodeImageSet.Spec.RegistryPolicy)
 		if err != nil {
 			log.FromContext(ctx).Error(err, "failed to get image status", "image", image)
 			continue
 		}
-		if status == ofenv1.WaitingForImageDownload || status == ofenv1.ImageDownloadFailed {
+		if status == ofenv1.WaitingForImageDownload {
 			images = append(images, image)
 		}
 	}
@@ -173,7 +173,7 @@ func (r *NodeImageSetReconciler) updateStatus(ctx context.Context, nodeImageSet
 
 	for _, image := range nodeImageSet.Spec.Images {
 		var errMsg string
-		status, errMsg, err := r.ImagePuller.GetImageStatus(ctx, nodeImageSet.Name, image)
+		status, errMsg, err := r.ImagePuller.GetImageStatus(ctx, nodeImageSet.Name, image, nodeImageSet.Spec.RegistryPolicy)
 		if err != nil {
 			return ctrl.Result{}, fmt.Errorf("failed to get image status for %s: %w", image, err)
 		}
@@ -242,7 +242,7 @@ func (r *NodeImageSetReconciler) calculateImageStatus(ctx context.Context, nis *
 	failed := 0
 
 	for _, image := range nis.Spec.Images {
-		status, _, err := r.ImagePuller.GetImageStatus(ctx, nis.Name, image)
+		status, _, err := r.ImagePuller.GetImageStatus(ctx, nis.Name, image, nis.Spec.RegistryPolicy)
 		if err != nil {
 			log.FromContext(ctx).Error(err, "failed to get image status", "image", image)
 			continue
@@ -252,6 +252,9 @@ func (r *NodeImageSetReconciler) calculateImageStatus(ctx context.Context, nis *
 			downloadedImage++
 		case ofenv1.ImageDownloadFailed:
 			failed++
+		case ofenv1.ImageDownloadTemporarilyFailed:
+			// Do not reflect ImageDownloadTemporarilyFailed in the status.
+			// It's a temporary error that resolves once the image is cached in the registry mirror.
 		case ofenv1.ImageDownloadInProgress:
 		default:
 		}

internal/imgmanager/image_puller.go

@@ -201,18 +201,13 @@ func (p *ImagePuller) PullImage(ctx context.Context, nodeImageSetName, ref strin
 
 	imageStatus.StopPulling()
 	if err != nil {
-		if errdefs.IsNotFound(err) && registryPolicy == ofenv1.RegistryPolicyMirrorOnly {
-			// If the image is not found and the policy is MirrorOnly, we treat it as a non-fatal error.
-			p.logger.Info("image not found in mirror registry, skipping pull", "image", ref)
-		} else {
-			imageStatus.SetError(err)
-		}
+		imageStatus.SetError(err)
 	}
 
 	return err
 }
 
-func (p *ImagePuller) GetImageStatus(ctx context.Context, nodeImageSetName, imageName string) (string, string, error) {
+func (p *ImagePuller) GetImageStatus(ctx context.Context, nodeImageSetName, imageName string, registryPolicy ofenv1.RegistryPolicy) (string, string, error) {
 	value, ok := p.status.Load(nodeImageSetName)
 	if !ok {
 		return "", "", nil
@@ -238,6 +233,12 @@ func (p *ImagePuller) GetImageStatus(ctx context.Context, nodeImageSetName, imag
 	}
 	imageErr := imageStatus.GetError()
 	if imageErr != nil {
+		// When using mirror-only registry policy, treat image not found errors as temporary failures.
+		// This handles cases where the image hasn't been cached in the registry mirror yet due to
+		// timing issues during the image pull process.
+		if errdefs.IsNotFound(imageErr) && registryPolicy == ofenv1.RegistryPolicyMirrorOnly {
+			return ofenv1.ImageDownloadTemporarilyFailed, imageErr.Error(), nil
+		}
 		return ofenv1.ImageDownloadFailed, imageErr.Error(), nil
 	}
 

internal/imgmanager/image_puller_test.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	eventtypes "github.com/containerd/containerd/api/events"
+	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/v2/core/events"
 	"github.com/containerd/typeurl/v2"
 	"github.com/go-logr/logr"
@@ -261,14 +262,14 @@ func TestImagePullerGetImageStatusStates(t *testing.T) {
 	puller.NewNodeImageSetStatus(testNodeImageSetName)
 
 	// Test WaitingForImageDownload state (initial state)
-	status, message, err := puller.GetImageStatus(ctx, testNodeImageSetName, testImageName)
+	status, message, err := puller.GetImageStatus(ctx, testNodeImageSetName, testImageName, ofenv1.RegistryPolicyDefault)
 	assert.NoError(t, err)
 	assert.Equal(t, ofenv1.WaitingForImageDownload, status)
 	assert.Empty(t, message)
 
 	// Test ImageDownloaded state
 	fakeClient.SetImageExists(testImageName, true)
-	status, message, err = puller.GetImageStatus(ctx, testNodeImageSetName, testImageName)
+	status, message, err = puller.GetImageStatus(ctx, testNodeImageSetName, testImageName, ofenv1.RegistryPolicyDefault)
 	assert.NoError(t, err)
 	assert.Equal(t, ofenv1.ImageDownloaded, status)
 	assert.Empty(t, message)
@@ -281,7 +282,7 @@ func TestImagePullerGetImageStatusStates(t *testing.T) {
 	imageStatus := nodeStatus.GetOrCreateImageStatus(testImageName)
 	imageStatus.SetImagePulling(true)
 
-	status, message, err = puller.GetImageStatus(ctx, testNodeImageSetName, testImageName)
+	status, message, err = puller.GetImageStatus(ctx, testNodeImageSetName, testImageName, ofenv1.RegistryPolicyDefault)
 	assert.NoError(t, err)
 	assert.Equal(t, ofenv1.ImageDownloadInProgress, status)
 	assert.Empty(t, message)
@@ -291,10 +292,28 @@ func TestImagePullerGetImageStatusStates(t *testing.T) {
 	testErr := fmt.Errorf("pull failed")
 	imageStatus.SetError(testErr)
 
-	status, message, err = puller.GetImageStatus(ctx, testNodeImageSetName, testImageName)
+	status, message, err = puller.GetImageStatus(ctx, testNodeImageSetName, testImageName, ofenv1.RegistryPolicyDefault)
 	assert.NoError(t, err)
 	assert.Equal(t, ofenv1.ImageDownloadFailed, status)
 	assert.Equal(t, "pull failed", message)
+
+	// Test ImageDownloadFailed state with a temporary error
+	imageStatus.ClearError()
+	imageStatus.SetImagePulling(false)
+	imageStatus.SetError(errdefs.ErrNotFound)
+	status, message, err = puller.GetImageStatus(ctx, testNodeImageSetName, testImageName, ofenv1.RegistryPolicyDefault)
+	assert.NoError(t, err)
+	assert.Equal(t, ofenv1.ImageDownloadFailed, status)
+	assert.Equal(t, "not found", message)
+
+	// Test ImageDownloadTemporarilyFailed state
+	imageStatus.ClearError()
+	imageStatus.SetImagePulling(false)
+	imageStatus.SetError(errdefs.ErrNotFound)
+	status, message, err = puller.GetImageStatus(ctx, testNodeImageSetName, testImageName, ofenv1.RegistryPolicyMirrorOnly)
+	assert.NoError(t, err)
+	assert.Equal(t, ofenv1.ImageDownloadTemporarilyFailed, status)
+	assert.Equal(t, "not found", message)
 }
 
 func TestImagePullerGetImageStatusNonexistentNodeImageSet(t *testing.T) {
@@ -306,7 +325,7 @@ func TestImagePullerGetImageStatusNonexistentNodeImageSet(t *testing.T) {
 
 	ctx := context.Background()
 
-	status, message, err := puller.GetImageStatus(ctx, "nonexistent", testImageName)
+	status, message, err := puller.GetImageStatus(ctx, "nonexistent", testImageName, ofenv1.RegistryPolicyDefault)
 	assert.NoError(t, err)
 	assert.Empty(t, status)
 	assert.Empty(t, message)