Skip to content

Commit

Permalink
fix: properly GC images supplied with both tag and digest
Browse files Browse the repository at this point in the history 
This is a follow-up fix for #7640

I noticed that image cleanup controller cleans up the images if
specified with both tag and digest.

The problem was incorrectly building image references in the expected
set of images, so they were incorrectly marked as unused.

Refactor the code to make the core part testable.

Signed-off-by: Andrey Smirnov <[email protected]>
  • Loading branch information
@smira
smira committed Aug 21, 2023
1 parent ccfa8de commit dc8361c
Show file tree
Hide file tree
Showing 3 changed files with 155 additions and 20 deletions.
50 changes: 31 additions & 19 deletions internal/app/machined/pkg/controllers/runtime/cri_image_gc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,16 +186,7 @@ func (ctrl *CRIImageGCController) Run(ctx context.Context, r controller.Runtime,
}

//nolint:gocyclo
func (ctrl *CRIImageGCController) cleanup(ctx context.Context, logger *zap.Logger, imageService images.Store, expectedImages []string) error {
logger.Debug("running image cleanup")

ctx = namespaces.WithNamespace(ctx, constants.SystemContainerdNamespace)

actualImages, err := imageService.List(ctx)
if err != nil {
return fmt.Errorf("error listing images: %w", err)
}

func buildExpectedImageNames(logger *zap.Logger, actualImages []images.Image, expectedImages []string) (map[string]struct{}, error) {
var parseErrors []error

expectedReferences := slices.Map(expectedImages, func(ref string) docker.Named {
Expand All @@ -206,19 +197,16 @@ func (ctrl *CRIImageGCController) cleanup(ctx context.Context, logger *zap.Logge
return res
})

if err = errors.Join(parseErrors...); err != nil {
return fmt.Errorf("error parsing expected images: %w", err)
if err := errors.Join(parseErrors...); err != nil {
return nil, fmt.Errorf("error parsing expected images: %w", err)
}

expectedImageNames := map[string]struct{}{}

// first pass: scan actualImages and expand expectedReferences with other non-canonical refs
for _, image := range actualImages {
var imageRef docker.Reference

imageRef, err = docker.ParseAnyReference(image.Name)
imageRef, err := docker.ParseAnyReference(image.Name)
if err != nil {
logger.Debug("failed to parse image referencer", zap.Error(err), zap.String("image", image.Name))
logger.Debug("failed to parse image reference", zap.Error(err), zap.String("image", image.Name))

continue
}
Expand All @@ -235,7 +223,7 @@ func (ctrl *CRIImageGCController) cleanup(ctx context.Context, logger *zap.Logge
if expectedTagged, ok := expectedRef.(docker.Tagged); ok && ref.Tag() == expectedTagged.Tag() {
// this is expected image by tag, inject other forms of the ref
expectedImageNames[digest] = struct{}{}
expectedImageNames[expectedRef.String()] = struct{}{}
expectedImageNames[expectedRef.Name()+":"+expectedTagged.Tag()] = struct{}{}
expectedImageNames[expectedRef.Name()+"@"+digest] = struct{}{}
}
}
Expand All @@ -248,12 +236,36 @@ func (ctrl *CRIImageGCController) cleanup(ctx context.Context, logger *zap.Logge
if expectedDigested, ok := expectedRef.(docker.Digested); ok && ref.Digest() == expectedDigested.Digest() {
// this is expected image by digest, inject other forms of the ref
expectedImageNames[digest] = struct{}{}
expectedImageNames[expectedRef.String()] = struct{}{}
expectedImageNames[expectedRef.Name()+"@"+digest] = struct{}{}

// if the image is also tagged, inject the tagged version of it
if expectedTagged, ok := expectedRef.(docker.Tagged); ok {
expectedImageNames[expectedRef.Name()+":"+expectedTagged.Tag()] = struct{}{}
}
}
}
}
}

return expectedImageNames, nil
}

func (ctrl *CRIImageGCController) cleanup(ctx context.Context, logger *zap.Logger, imageService images.Store, expectedImages []string) error {
logger.Debug("running image cleanup")

ctx = namespaces.WithNamespace(ctx, constants.SystemContainerdNamespace)

actualImages, err := imageService.List(ctx)
if err != nil {
return fmt.Errorf("error listing images: %w", err)
}

// first pass: scan actualImages and expand expectedReferences with other non-canonical refs
expectedImageNames, err := buildExpectedImageNames(logger, actualImages, expectedImages)
if err != nil {
return err
}

// second pass, drop whatever is not expected
for _, image := range actualImages {
_, shouldKeep := expectedImageNames[image.Name]
Expand Down
117 changes: 116 additions & 1 deletion internal/app/machined/pkg/controllers/runtime/cri_image_gc_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package runtime_test
import (
"context"
"reflect"
"sort"
"sync"
"testing"
"time"
Expand All @@ -15,9 +16,13 @@ import (
"github.com/containerd/containerd/images"
"github.com/opencontainers/go-digest"
v1 "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/siderolabs/gen/maps"
"github.com/siderolabs/gen/slices"
"github.com/siderolabs/go-retry/retry"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
"go.uber.org/zap/zaptest"

"github.com/siderolabs/talos/internal/app/machined/pkg/controllers/ctest"
runtimectrl "github.com/siderolabs/talos/internal/app/machined/pkg/controllers/runtime"
Expand Down Expand Up @@ -161,7 +166,7 @@ func (suite *CRIImageGCSuite) TestReconcile() {
suite.Require().NoError(suite.State().Create(suite.Ctx(), kubelet))

etcd := etcd.NewSpec(etcd.NamespaceName, etcd.SpecID)
etcd.TypedSpec().Image = "registry.io/org/image2@sha256:2f794176e9bd8a28501fa185693dc1073013a048c51585022ebce4f84b469db8"
etcd.TypedSpec().Image = "registry.io/org/image2:v3.5.9@sha256:2f794176e9bd8a28501fa185693dc1073013a048c51585022ebce4f84b469db8"
suite.Require().NoError(suite.State().Create(suite.Ctx(), etcd))

expectedImages := slices.Map(storedImages[2:7], func(i images.Image) string { return i.Name })
Expand All @@ -179,3 +184,113 @@ func (suite *CRIImageGCSuite) TestReconcile() {
return retry.ExpectedErrorf("images don't match: expected %v actual %v", expectedImages, actualImages)
}))
}

func TestBuildExpectedImageNames(t *testing.T) {
actualImages := []images.Image{
{
Name: "registry.io/org/image1:v1.3.5@sha256:6b094bd0b063a1172eec7da249eccbb48cc48333800569363d67c747960cfa0a",
Target: v1.Descriptor{
Digest: must(digest.Parse("sha256:6b094bd0b063a1172eec7da249eccbb48cc48333800569363d67c747960cfa0a")),
},
},
{
Name: "sha256:6b094bd0b063a1172eec7da249eccbb48cc48333800569363d67c747960cfa0a",
Target: v1.Descriptor{
Digest: must(digest.Parse("sha256:6b094bd0b063a1172eec7da249eccbb48cc48333800569363d67c747960cfa0a")),
},
},
{
Name: "registry.io/org/image1:v1.3.7",
Target: v1.Descriptor{
Digest: must(digest.Parse("sha256:7051a34bcd2522e58a2291d1aa065667f225fd07e4445590b091e86c6799b135")),
},
},
{
Name: "registry.io/org/image1@sha256:7051a34bcd2522e58a2291d1aa065667f225fd07e4445590b091e86c6799b135",
Target: v1.Descriptor{
Digest: must(digest.Parse("sha256:7051a34bcd2522e58a2291d1aa065667f225fd07e4445590b091e86c6799b135")),
},
},
{
Name: "sha256:7051a34bcd2522e58a2291d1aa065667f225fd07e4445590b091e86c6799b135",
Target: v1.Descriptor{
Digest: must(digest.Parse("sha256:7051a34bcd2522e58a2291d1aa065667f225fd07e4445590b091e86c6799b135")),
},
},
{
Name: "registry.io/org/image1:v1.3.8",
Target: v1.Descriptor{
Digest: must(digest.Parse("sha256:fd03335dd2e7163e5e36e933a0c735d7fec6f42b33ddafad0bc54f333e4a23c0")),
},
},
{
Name: "registry.io/org/image2@sha256:2f794176e9bd8a28501fa185693dc1073013a048c51585022ebce4f84b469db8",
Target: v1.Descriptor{
Digest: must(digest.Parse("sha256:2f794176e9bd8a28501fa185693dc1073013a048c51585022ebce4f84b469db8")),
},
},
}

logger := zaptest.NewLogger(t)

for _, test := range []struct {
name string
expectedImages []string

expectedImageNames []string
}{
{
name: "empty",
},
{
name: "by tag",
expectedImages: []string{
"registry.io/org/image1:v1.3.7",
},
expectedImageNames: []string{
"registry.io/org/image1:v1.3.7",
"registry.io/org/image1@sha256:7051a34bcd2522e58a2291d1aa065667f225fd07e4445590b091e86c6799b135",
"sha256:7051a34bcd2522e58a2291d1aa065667f225fd07e4445590b091e86c6799b135",
},
},
{
name: "by digest",
expectedImages: []string{
"registry.io/org/image1@sha256:7051a34bcd2522e58a2291d1aa065667f225fd07e4445590b091e86c6799b135",
},
expectedImageNames: []string{
"registry.io/org/image1@sha256:7051a34bcd2522e58a2291d1aa065667f225fd07e4445590b091e86c6799b135",
"sha256:7051a34bcd2522e58a2291d1aa065667f225fd07e4445590b091e86c6799b135",
},
},
{
name: "by digest and tag",
expectedImages: []string{
"registry.io/org/image1:v1.3.7@sha256:7051a34bcd2522e58a2291d1aa065667f225fd07e4445590b091e86c6799b135",
},
expectedImageNames: []string{
"registry.io/org/image1:v1.3.7",
"registry.io/org/image1@sha256:7051a34bcd2522e58a2291d1aa065667f225fd07e4445590b091e86c6799b135",
"sha256:7051a34bcd2522e58a2291d1aa065667f225fd07e4445590b091e86c6799b135",
},
},
{
name: "not found",
expectedImages: []string{
"registry.io/org/image1:v1.3.9",
},
},
} {
t.Run(test.name, func(t *testing.T) {
expectedImages, err := runtimectrl.BuildExpectedImageNames(logger, actualImages, test.expectedImages)
require.NoError(t, err)

expectedImageNames := maps.Keys(expectedImages)

sort.Strings(test.expectedImageNames)
sort.Strings(expectedImageNames)

assert.Equal(t, test.expectedImageNames, expectedImageNames)
})
}
}
8 changes: 8 additions & 0 deletions internal/app/machined/pkg/controllers/runtime/export_test.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/.

package runtime

// BuildExpectedImageNames is exported for testing.
var BuildExpectedImageNames = buildExpectedImageNames

0 comments on commit dc8361c

Please sign in to comment.