update the image status when start pulling images & compare(without …

…defaultDomain and officialRepoName) name and tag of image Signed-off-by: liuzhenwei <dui_zhang@163.com> add ut for ContainsImage Signed-off-by: liuzhenwei <dui_zhang@163.com>
openkruise · May 10, 2023 · 3a67439 · 3a67439
1 parent 7f5046d
commit 3a67439
Show file tree

Hide file tree

Showing 4 changed files with 98 additions and 36 deletions.
diff --git a/pkg/daemon/criruntime/imageruntime/helpers.go b/pkg/daemon/criruntime/imageruntime/helpers.go
@@ -20,15 +20,15 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"strings"
 
 	dockermessage "github.com/docker/docker/pkg/jsonmessage"
-	daemonutil "github.com/openkruise/kruise/pkg/daemon/util"
-	"github.com/openkruise/kruise/pkg/util"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/klog/v2"
 	"k8s.io/kubernetes/pkg/credentialprovider"
 	credentialprovidersecrets "k8s.io/kubernetes/pkg/credentialprovider/secrets"
+
+	daemonutil "github.com/openkruise/kruise/pkg/daemon/util"
+	"github.com/openkruise/kruise/pkg/util"
 )
 
 var (
@@ -230,31 +230,11 @@ func (r *imagePullStatusReader) mainloop() {
 
 func (c ImageInfo) ContainsImage(name string, tag string) bool {
 	for _, repoTag := range c.RepoTags {
-		imageRepo, imageTag := parseRepositoryTag(repoTag)
+		// Ref: https://github.com/openkruise/kruise/issues/1273
+		imageRepo, imageTag, _ := daemonutil.NormalizeImageRefToNameTag(repoTag)
 		if imageRepo == name && imageTag == tag {
 			return true
 		}
 	}
 	return false
 }
-
-// parseRepositoryTag gets a repos name and returns the right reposName + tag|digest
-// The tag can be confusing because of a port in a repository name.
-//
-//	Ex: localhost.localdomain:5000/samalba/hipache:latest
-//	Digest ex: localhost:5000/foo/bar@sha256:bc8813ea7b3603864987522f02a76101c17ad122e1c46d790efc0fca78ca7bfb
-func parseRepositoryTag(repos string) (string, string) {
-	n := strings.Index(repos, "@")
-	if n >= 0 {
-		parts := strings.Split(repos, "@")
-		return parts[0], parts[1]
-	}
-	n = strings.LastIndex(repos, ":")
-	if n < 0 {
-		return repos, ""
-	}
-	if tag := repos[n+1:]; !strings.Contains(tag, "/") {
-		return repos[:n], tag
-	}
-	return repos, ""
-}
diff --git a/pkg/daemon/criruntime/imageruntime/helpers_test.go b/pkg/daemon/criruntime/imageruntime/helpers_test.go
@@ -117,3 +117,76 @@ func TestMatchRegistryAuths(t *testing.T) {
 		})
 	}
 }
+
+func TestContainsImage(t *testing.T) {
+	cases := []struct {
+		name       string
+		ImageName  string
+		Tag        string
+		ImageInfos []ImageInfo
+		Expect     bool
+	}{
+		{
+			name:      "test_nginx",
+			ImageName: "nginx",
+			Tag:       "latest",
+			ImageInfos: []ImageInfo{{
+				RepoTags: []string{"docker.io/library/nginx:latest"},
+			},
+			},
+			Expect: true,
+		},
+		{
+			name:      "test_test/nginx:1.0",
+			ImageName: "test/nginx",
+			Tag:       "1.0",
+			ImageInfos: []ImageInfo{{
+				RepoTags: []string{"docker.io/test/nginx:1.0"},
+			},
+			},
+			Expect: true,
+		},
+		{
+			name:      "test_test/nginx:1.0_false",
+			ImageName: "test/nginx",
+			Tag:       "1.0",
+			ImageInfos: []ImageInfo{{
+				RepoTags: []string{"docker.io/library/nginx:1.0"},
+			},
+			},
+			Expect: false,
+		},
+
+		{
+			name:      "test_docker.io/library/nginx",
+			ImageName: "docker.io/library/nginx",
+			Tag:       "latest",
+			ImageInfos: []ImageInfo{{
+				RepoTags: []string{"docker.io/library/nginx:latest"},
+			},
+			},
+			Expect: false,
+		},
+		{
+			name:      "test_1.1.1.1:8080/test/nginx",
+			ImageName: "1.1.1.1:8080/test/nginx",
+			Tag:       "latest",
+			ImageInfos: []ImageInfo{{
+				RepoTags: []string{"1.1.1.1:8080/test/nginx:latest"},
+			},
+			},
+			Expect: true,
+		},
+	}
+
+	for _, cs := range cases {
+		t.Run(cs.name, func(t *testing.T) {
+			for _, info := range cs.ImageInfos {
+				res := info.ContainsImage(cs.ImageName, cs.Tag)
+				if res != cs.Expect {
+					t.Fatalf("ContainsImage failed")
+				}
+			}
+		})
+	}
+}
diff --git a/pkg/daemon/imagepuller/imagepuller_controller.go b/pkg/daemon/imagepuller/imagepuller_controller.go
@@ -24,13 +24,6 @@ import (
 	"reflect"
 	"time"
 
-	appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1"
-	"github.com/openkruise/kruise/pkg/client"
-	kruiseclient "github.com/openkruise/kruise/pkg/client/clientset/versioned"
-	listersalpha1 "github.com/openkruise/kruise/pkg/client/listers/apps/v1alpha1"
-	daemonoptions "github.com/openkruise/kruise/pkg/daemon/options"
-	daemonutil "github.com/openkruise/kruise/pkg/daemon/util"
-	utilimagejob "github.com/openkruise/kruise/pkg/util/imagejob"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -44,6 +37,14 @@ import (
 	"k8s.io/client-go/tools/reference"
 	"k8s.io/client-go/util/workqueue"
 	"k8s.io/klog/v2"
+
+	appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1"
+	"github.com/openkruise/kruise/pkg/client"
+	kruiseclient "github.com/openkruise/kruise/pkg/client/clientset/versioned"
+	listersalpha1 "github.com/openkruise/kruise/pkg/client/listers/apps/v1alpha1"
+	daemonoptions "github.com/openkruise/kruise/pkg/daemon/options"
+	daemonutil "github.com/openkruise/kruise/pkg/daemon/util"
+	utilimagejob "github.com/openkruise/kruise/pkg/util/imagejob"
 )
 
 type Controller struct {

diff --git a/pkg/daemon/imagepuller/imagepuller_worker.go b/pkg/daemon/imagepuller/imagepuller_worker.go
@@ -22,15 +22,16 @@ import (
 	"sync"
 	"time"
 
-	appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1"
-	runtimeimage "github.com/openkruise/kruise/pkg/daemon/criruntime/imageruntime"
-	daemonutil "github.com/openkruise/kruise/pkg/daemon/util"
-	"github.com/openkruise/kruise/pkg/util"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/klog/v2"
+
+	appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1"
+	runtimeimage "github.com/openkruise/kruise/pkg/daemon/criruntime/imageruntime"
+	daemonutil "github.com/openkruise/kruise/pkg/daemon/util"
+	"github.com/openkruise/kruise/pkg/util"
 )
 
 const (
@@ -324,6 +325,13 @@ func (w *pullWorker) Run() {
 		StartTime: &startTime,
 		Version:   w.tagSpec.Version,
 	}
+
+	// We should update the image status when we start pulling images,
+	// which can meet the scenario that some large size images cannot return the result from CRI.PullImage within 60s. For one reason:
+	// For nodeimage controller will mark image:tag task failed (not responded for a long time) if daemon does not report status in 60s.
+	// Ref: https://github.com/openkruise/kruise/issues/1273
+	w.statusUpdater.UpdateStatus(newStatus)
+
 	defer func() {
 		cost := time.Since(startTime.Time)
 		if newStatus.Phase == appsv1alpha1.ImagePhaseFailed {