From 273c2bb168d1733ebcbe7418ca1232237f3dc603 Mon Sep 17 00:00:00 2001 From: Claudiu Belu Date: Mon, 19 Apr 2021 20:51:42 +0000 Subject: [PATCH 1/5] tests: Prepull images used in tests Most of the tests are pulling and deleting the same test images, which can be quite inefficient, especially on Windows nodes, where the images are larger than the Linux ones (a nanoserver Container image is ~250MB in size). We can instead pull them only once, and reuse them. This will reduce the test run time on Windows considerably. Additionally, some of the test images are currently hosted on dockerhub (busybox image), which has introduced image ratelimiting in November 2020, which means that test runners could potentially hit that limit faster with the current implementation. This will reduce that risk. Some tests are specifically deleting images, so we always have to ensure that they are pulled. Signed-off-by: Claudiu Belu --- integration/addition_gids_test.go | 8 ++------ integration/common.go | 20 +++++++++++++++++++ integration/container_log_test.go | 16 ++++----------- integration/container_stop_test.go | 16 ++++----------- .../container_without_image_ref_test.go | 8 ++------ integration/containerd_image_test.go | 9 ++------- integration/imagefs_info_test.go | 8 ++------ integration/pod_dualstack_test.go | 8 ++------ integration/pod_hostname_test.go | 8 ++------ integration/restart_test.go | 6 +----- integration/truncindex_test.go | 7 ++----- integration/volume_copy_up_test.go | 9 ++------- 12 files changed, 45 insertions(+), 78 deletions(-) diff --git a/integration/addition_gids_test.go b/integration/addition_gids_test.go index 7674fe0ed..76eebbb7d 100644 --- a/integration/addition_gids_test.go +++ b/integration/addition_gids_test.go @@ -49,12 +49,8 @@ func TestAdditionalGids(t *testing.T) { testImage = GetImage(BusyBox) containerName = "test-container" ) - t.Logf("Pull test image %q", testImage) - img, err := imageService.PullImage(&runtime.ImageSpec{Image: testImage}, nil, sbConfig) - require.NoError(t, err) - defer func() { - assert.NoError(t, imageService.RemoveImage(&runtime.ImageSpec{Image: img})) - }() + + EnsureImageExists(t, testImage) t.Log("Create a container to print id") cnConfig := ContainerConfig( diff --git a/integration/common.go b/integration/common.go index 24cb7de82..ac657eb91 100644 --- a/integration/common.go +++ b/integration/common.go @@ -21,10 +21,13 @@ package integration import ( "fmt" "io/ioutil" + "testing" "github.com/pelletier/go-toml" "github.com/sirupsen/logrus" + "github.com/stretchr/testify/require" cri "k8s.io/cri-api/pkg/apis" + runtime "k8s.io/cri-api/pkg/apis/runtime/v1alpha2" ) // ImageList holds public image references @@ -99,3 +102,20 @@ func initImageMap(imageList ImageList) map[int]string { func GetImage(image int) string { return imageMap[image] } + +// EnsureImageExists pulls the given image, ensures that no error was encountered +// while pulling it. +func EnsureImageExists(t *testing.T, imageName string) string { + img, err := imageService.ImageStatus(&runtime.ImageSpec{Image: imageName}) + require.NoError(t, err) + if img != nil { + t.Logf("Image %q already exists, not pulling.", imageName) + return img.Id + } + + t.Logf("Pull test image %q", imageName) + imgID, err := imageService.PullImage(&runtime.ImageSpec{Image: imageName}, nil, nil) + require.NoError(t, err) + + return imgID +} diff --git a/integration/container_log_test.go b/integration/container_log_test.go index 56aaa443e..6c226c13b 100644 --- a/integration/container_log_test.go +++ b/integration/container_log_test.go @@ -52,12 +52,8 @@ func TestContainerLogWithoutTailingNewLine(t *testing.T) { testImage = GetImage(BusyBox) containerName = "test-container" ) - t.Logf("Pull test image %q", testImage) - img, err := imageService.PullImage(&runtime.ImageSpec{Image: testImage}, nil, sbConfig) - require.NoError(t, err) - defer func() { - assert.NoError(t, imageService.RemoveImage(&runtime.ImageSpec{Image: img})) - }() + + EnsureImageExists(t, testImage) t.Log("Create a container with log path") cnConfig := ContainerConfig( @@ -112,12 +108,8 @@ func TestLongContainerLog(t *testing.T) { testImage = GetImage(BusyBox) containerName = "test-container" ) - t.Logf("Pull test image %q", testImage) - img, err := imageService.PullImage(&runtime.ImageSpec{Image: testImage}, nil, sbConfig) - require.NoError(t, err) - defer func() { - assert.NoError(t, imageService.RemoveImage(&runtime.ImageSpec{Image: img})) - }() + + EnsureImageExists(t, testImage) t.Log("Create a container with log path") config, err := CRIConfig() diff --git a/integration/container_stop_test.go b/integration/container_stop_test.go index 6c5c8cec6..fd5d6eb12 100644 --- a/integration/container_stop_test.go +++ b/integration/container_stop_test.go @@ -46,12 +46,8 @@ func TestSharedPidMultiProcessContainerStop(t *testing.T) { testImage = GetImage(BusyBox) containerName = "test-container" ) - t.Logf("Pull test image %q", testImage) - img, err := imageService.PullImage(&runtime.ImageSpec{Image: testImage}, nil, sbConfig) - require.NoError(t, err) - defer func() { - assert.NoError(t, imageService.RemoveImage(&runtime.ImageSpec{Image: img})) - }() + + EnsureImageExists(t, testImage) t.Log("Create a multi-process container") cnConfig := ContainerConfig( @@ -90,12 +86,8 @@ func TestContainerStopCancellation(t *testing.T) { testImage = GetImage(BusyBox) containerName = "test-container" ) - t.Logf("Pull test image %q", testImage) - img, err := imageService.PullImage(&runtime.ImageSpec{Image: testImage}, nil, sbConfig) - require.NoError(t, err) - defer func() { - assert.NoError(t, imageService.RemoveImage(&runtime.ImageSpec{Image: img})) - }() + + EnsureImageExists(t, testImage) t.Log("Create a container which traps sigterm") cnConfig := ContainerConfig( diff --git a/integration/container_without_image_ref_test.go b/integration/container_without_image_ref_test.go index e284bf680..4be07b147 100644 --- a/integration/container_without_image_ref_test.go +++ b/integration/container_without_image_ref_test.go @@ -41,12 +41,8 @@ func TestContainerLifecycleWithoutImageRef(t *testing.T) { testImage = GetImage(BusyBox) containerName = "test-container" ) - t.Log("Pull test image") - img, err := imageService.PullImage(&runtime.ImageSpec{Image: testImage}, nil, sbConfig) - require.NoError(t, err) - defer func() { - assert.NoError(t, imageService.RemoveImage(&runtime.ImageSpec{Image: img})) - }() + + img := EnsureImageExists(t, testImage) t.Log("Create test container") cnConfig := ContainerConfig( diff --git a/integration/containerd_image_test.go b/integration/containerd_image_test.go index 9d81b38ef..9907a1d36 100644 --- a/integration/containerd_image_test.go +++ b/integration/containerd_image_test.go @@ -185,13 +185,8 @@ func TestContainerdImageInOtherNamespaces(t *testing.T) { } require.NoError(t, Consistently(checkImage, 100*time.Millisecond, time.Second)) - sbConfig := PodSandboxConfig("sandbox", "test") - t.Logf("pull the image into cri plugin") - id, err := imageService.PullImage(&runtime.ImageSpec{Image: testImage}, nil, sbConfig) - require.NoError(t, err) - defer func() { - assert.NoError(t, imageService.RemoveImage(&runtime.ImageSpec{Image: id})) - }() + PodSandboxConfig("sandbox", "test") + EnsureImageExists(t, testImage) t.Logf("cri plugin should see the image now") img, err := imageService.ImageStatus(&runtime.ImageSpec{Image: testImage}) diff --git a/integration/imagefs_info_test.go b/integration/imagefs_info_test.go index f64653d78..dc9210c10 100644 --- a/integration/imagefs_info_test.go +++ b/integration/imagefs_info_test.go @@ -33,12 +33,8 @@ func TestImageFSInfo(t *testing.T) { config := PodSandboxConfig("running-pod", "imagefs") t.Logf("Pull an image to make sure image fs is not empty") - img, err := imageService.PullImage(&runtime.ImageSpec{Image: GetImage(BusyBox)}, nil, config) - require.NoError(t, err) - defer func() { - err := imageService.RemoveImage(&runtime.ImageSpec{Image: img}) - assert.NoError(t, err) - }() + EnsureImageExists(t, GetImage(BusyBox)) + t.Logf("Create a sandbox to make sure there is an active snapshot") sb, err := runtimeService.RunPodSandbox(config, *runtimeHandler) require.NoError(t, err) diff --git a/integration/pod_dualstack_test.go b/integration/pod_dualstack_test.go index 95c22b5ca..339460ab8 100644 --- a/integration/pod_dualstack_test.go +++ b/integration/pod_dualstack_test.go @@ -50,12 +50,8 @@ func TestPodDualStack(t *testing.T) { testImage = GetImage(BusyBox) containerName = "test-container" ) - t.Logf("Pull test image %q", testImage) - img, err := imageService.PullImage(&runtime.ImageSpec{Image: testImage}, nil, sbConfig) - require.NoError(t, err) - defer func() { - assert.NoError(t, imageService.RemoveImage(&runtime.ImageSpec{Image: img})) - }() + + EnsureImageExists(t, testImage) t.Log("Create a container to print env") cnConfig := ContainerConfig( diff --git a/integration/pod_hostname_test.go b/integration/pod_hostname_test.go index 38a02c4b7..dcebee2b5 100644 --- a/integration/pod_hostname_test.go +++ b/integration/pod_hostname_test.go @@ -86,12 +86,8 @@ func TestPodHostname(t *testing.T) { testImage = GetImage(BusyBox) containerName = "test-container" ) - t.Logf("Pull test image %q", testImage) - img, err := imageService.PullImage(&runtime.ImageSpec{Image: testImage}, nil, sbConfig) - require.NoError(t, err) - defer func() { - assert.NoError(t, imageService.RemoveImage(&runtime.ImageSpec{Image: img})) - }() + + EnsureImageExists(t, testImage) t.Log("Create a container to print env") cnConfig := ContainerConfig( diff --git a/integration/restart_test.go b/integration/restart_test.go index 2ba89fcfe..38dc8693e 100644 --- a/integration/restart_test.go +++ b/integration/restart_test.go @@ -135,11 +135,7 @@ func TestContainerdRestart(t *testing.T) { t.Logf("Pull test images") for _, image := range []string{GetImage(BusyBox), GetImage(Alpine)} { - img, err := imageService.PullImage(&runtime.ImageSpec{Image: image}, nil, nil) - require.NoError(t, err) - defer func() { - assert.NoError(t, imageService.RemoveImage(&runtime.ImageSpec{Image: img})) - }() + EnsureImageExists(t, image) } imagesBeforeRestart, err := imageService.ListImages(nil) assert.NoError(t, err) diff --git a/integration/truncindex_test.go b/integration/truncindex_test.go index 67cddeb1e..be9ebc9d6 100644 --- a/integration/truncindex_test.go +++ b/integration/truncindex_test.go @@ -35,12 +35,9 @@ func TestTruncIndex(t *testing.T) { t.Logf("Pull an image") var appImage = GetImage(BusyBox) - imgID, err := imageService.PullImage(&runtimeapi.ImageSpec{Image: appImage}, nil, sbConfig) - require.NoError(t, err) + + imgID := EnsureImageExists(t, appImage) imgTruncID := genTruncIndex(imgID) - defer func() { - assert.NoError(t, imageService.RemoveImage(&runtimeapi.ImageSpec{Image: imgTruncID})) - }() t.Logf("Get image status by truncindex, truncID: %s", imgTruncID) res, err := imageService.ImageStatus(&runtimeapi.ImageSpec{Image: imgTruncID}) diff --git a/integration/volume_copy_up_test.go b/integration/volume_copy_up_test.go index d6840a516..c03212772 100644 --- a/integration/volume_copy_up_test.go +++ b/integration/volume_copy_up_test.go @@ -26,7 +26,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - runtime "k8s.io/cri-api/pkg/apis/runtime/v1alpha2" ) func TestVolumeCopyUp(t *testing.T) { @@ -44,9 +43,7 @@ func TestVolumeCopyUp(t *testing.T) { assert.NoError(t, runtimeService.RemovePodSandbox(sb)) }() - t.Logf("Pull test image") - _, err = imageService.PullImage(&runtime.ImageSpec{Image: testImage}, nil, sbConfig) - require.NoError(t, err) + EnsureImageExists(t, testImage) t.Logf("Create a container with volume-copy-up test image") cnConfig := ContainerConfig( @@ -106,9 +103,7 @@ func TestVolumeOwnership(t *testing.T) { assert.NoError(t, runtimeService.RemovePodSandbox(sb)) }() - t.Logf("Pull test image") - _, err = imageService.PullImage(&runtime.ImageSpec{Image: testImage}, nil, sbConfig) - require.NoError(t, err) + EnsureImageExists(t, testImage) t.Logf("Create a container with volume-ownership test image") cnConfig := ContainerConfig( From 98f5922b5e27f5e1d4e398091c6f2c24eea203dd Mon Sep 17 00:00:00 2001 From: Iceber Gu Date: Fri, 30 Apr 2021 23:32:04 +0800 Subject: [PATCH 2/5] plugin: optimize the check for the last registration Signed-off-by: Iceber Gu --- plugin/plugin.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/plugin/plugin.go b/plugin/plugin.go index 2674edeb1..1894afe8c 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -171,15 +171,11 @@ func Register(r *Registration) { panic(err) } - var last bool for _, requires := range r.Requires { - if requires == "*" { - last = true + if requires == "*" && len(r.Requires) != 1 { + panic(ErrInvalidRequires) } } - if last && len(r.Requires) != 1 { - panic(ErrInvalidRequires) - } register.r = append(register.r, r) } From 8014d9fee0e2a758d34af977f1ea6a1b6932a098 Mon Sep 17 00:00:00 2001 From: Aditi Sharma Date: Mon, 1 Mar 2021 18:12:19 +0530 Subject: [PATCH 3/5] Skip TLS verification for localhost Signed-off-by: Aditi Sharma --- pkg/cri/server/image_pull.go | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/pkg/cri/server/image_pull.go b/pkg/cri/server/image_pull.go index 2165b3438..20a6d7168 100644 --- a/pkg/cri/server/image_pull.go +++ b/pkg/cri/server/image_pull.go @@ -337,6 +337,9 @@ func (c *criService) registryHosts(auth *runtime.AuthConfig) docker.RegistryHost if err != nil { return nil, errors.Wrapf(err, "get TLSConfig for registry %q", e) } + } else if isLocalHost(host) && u.Scheme == "http" { + // Skipping TLS verification for localhost + transport.TLSClientConfig.InsecureSkipVerify = true } if auth == nil && config.Auth != nil { @@ -366,15 +369,26 @@ func (c *criService) registryHosts(auth *runtime.AuthConfig) docker.RegistryHost // defaultScheme returns the default scheme for a registry host. func defaultScheme(host string) string { - if h, _, err := net.SplitHostPort(host); err == nil { - host = h - } - if host == "localhost" || host == "127.0.0.1" || host == "::1" { + if isLocalHost(host) { return "http" } return "https" } +// isLocalHost checks if the registry host is local. +func isLocalHost(host string) bool { + if h, _, err := net.SplitHostPort(host); err == nil { + host = h + } + + if host == "localhost" { + return true + } + + ip := net.ParseIP(host) + return ip.IsLoopback() +} + // addDefaultScheme returns the endpoint with default scheme func addDefaultScheme(endpoint string) (string, error) { if strings.Contains(endpoint, "://") { From e37ddafab457596bc55151cd5bb34a77919c85ac Mon Sep 17 00:00:00 2001 From: Iceber Gu Date: Fri, 7 May 2021 18:33:10 +0800 Subject: [PATCH 4/5] metadata: modify NewLeaseManager to return leases.Manager Signed-off-by: Iceber Gu --- metadata/leases.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/metadata/leases.go b/metadata/leases.go index 60da06b0f..c559fca85 100644 --- a/metadata/leases.go +++ b/metadata/leases.go @@ -33,22 +33,22 @@ import ( bolt "go.etcd.io/bbolt" ) -// LeaseManager manages the create/delete lifecycle of leases +// leaseManager manages the create/delete lifecycle of leases // and also returns existing leases -type LeaseManager struct { +type leaseManager struct { db *DB } // NewLeaseManager creates a new lease manager for managing leases using // the provided database transaction. -func NewLeaseManager(db *DB) *LeaseManager { - return &LeaseManager{ +func NewLeaseManager(db *DB) leases.Manager { + return &leaseManager{ db: db, } } // Create creates a new lease using the provided lease -func (lm *LeaseManager) Create(ctx context.Context, opts ...leases.Opt) (leases.Lease, error) { +func (lm *leaseManager) Create(ctx context.Context, opts ...leases.Opt) (leases.Lease, error) { var l leases.Lease for _, opt := range opts { if err := opt(&l); err != nil { @@ -102,7 +102,7 @@ func (lm *LeaseManager) Create(ctx context.Context, opts ...leases.Opt) (leases. } // Delete deletes the lease with the provided lease ID -func (lm *LeaseManager) Delete(ctx context.Context, lease leases.Lease, _ ...leases.DeleteOpt) error { +func (lm *leaseManager) Delete(ctx context.Context, lease leases.Lease, _ ...leases.DeleteOpt) error { namespace, err := namespaces.NamespaceRequired(ctx) if err != nil { return err @@ -127,7 +127,7 @@ func (lm *LeaseManager) Delete(ctx context.Context, lease leases.Lease, _ ...lea } // List lists all active leases -func (lm *LeaseManager) List(ctx context.Context, fs ...string) ([]leases.Lease, error) { +func (lm *leaseManager) List(ctx context.Context, fs ...string) ([]leases.Lease, error) { namespace, err := namespaces.NamespaceRequired(ctx) if err != nil { return nil, err @@ -183,7 +183,7 @@ func (lm *LeaseManager) List(ctx context.Context, fs ...string) ([]leases.Lease, } // AddResource references the resource by the provided lease. -func (lm *LeaseManager) AddResource(ctx context.Context, lease leases.Lease, r leases.Resource) error { +func (lm *leaseManager) AddResource(ctx context.Context, lease leases.Lease, r leases.Resource) error { namespace, err := namespaces.NamespaceRequired(ctx) if err != nil { return err @@ -212,7 +212,7 @@ func (lm *LeaseManager) AddResource(ctx context.Context, lease leases.Lease, r l } // DeleteResource dereferences the resource by the provided lease. -func (lm *LeaseManager) DeleteResource(ctx context.Context, lease leases.Lease, r leases.Resource) error { +func (lm *leaseManager) DeleteResource(ctx context.Context, lease leases.Lease, r leases.Resource) error { namespace, err := namespaces.NamespaceRequired(ctx) if err != nil { return err @@ -250,7 +250,7 @@ func (lm *LeaseManager) DeleteResource(ctx context.Context, lease leases.Lease, } // ListResources lists all the resources referenced by the lease. -func (lm *LeaseManager) ListResources(ctx context.Context, lease leases.Lease) ([]leases.Resource, error) { +func (lm *leaseManager) ListResources(ctx context.Context, lease leases.Lease) ([]leases.Resource, error) { namespace, err := namespaces.NamespaceRequired(ctx) if err != nil { return nil, err From fc4da9728e6231560411292ca9a2f6a591f64e57 Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Thu, 13 May 2021 09:50:57 -0700 Subject: [PATCH 5/5] Pin integration test image for alpine The latest tag is no longer available for alpine, pin to the latest version rather than using latest Signed-off-by: Derek McGowan --- integration/client/container_linux_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/client/container_linux_test.go b/integration/client/container_linux_test.go index 90f1b635c..e18381b04 100644 --- a/integration/client/container_linux_test.go +++ b/integration/client/container_linux_test.go @@ -55,7 +55,7 @@ import ( "golang.org/x/sys/unix" ) -const testUserNSImage = "mirror.gcr.io/library/alpine:latest" +const testUserNSImage = "mirror.gcr.io/library/alpine:3.13" // TestRegressionIssue4769 verifies the number of task exit events. //