Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: imgutil/local: Add a warning message when saving a local image and containerd storage is enable #284 #285

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions local/local_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,16 @@ const someSHA = "sha256:aec070645fe53ee3b3763059376134f058cc337247c978add178b6cc

var localTestRegistry *h.DockerRegistry

type TestLogger struct {
WarnCalled bool
Message string
}

func (logger *TestLogger) Warn(msg string) {
logger.WarnCalled = true
logger.Message = msg
}

func TestLocal(t *testing.T) {
localTestRegistry = h.NewDockerRegistry()
localTestRegistry.Start(t)
Expand Down Expand Up @@ -2028,6 +2038,52 @@ func testImage(t *testing.T, when spec.G, it spec.S) {
h.AssertError(t, err, "daemon response")
})
})
when("logger is set", func() {
when("containerd is used", func() {
testLogger := &TestLogger{}
dockerClient := h.DockerCli(t)
imgName := "test-image"

img, err := local.NewImage(imgName, dockerClient, local.WithLogger(testLogger))
if err != nil {
t.Fatalf("Failed to create image: %v", err)
}
err = img.Save()
if err != nil {
t.Fatalf("Failed to save image: %v", err)
}

defer func() {
if err := h.DockerRmi(dockerClient, imgName); err != nil {
t.Errorf("Failed to remove image %s: %v", imgName, err)
}
}()

isContainerd := func(cli local.DockerClient) bool {
info, err := cli.Info(context.Background())
if err != nil {
return false
}
for _, driverStatus := range info.DriverStatus {
if driverStatus[0] == "driver-type" && driverStatus[1] == "io.containerd.snapshotter.v1" {
return true
}
}
return false
}(dockerClient)
if isContainerd {
if !testLogger.WarnCalled {
t.Error("Expected warning to be logged,but it was not.")
t.Errorf(testLogger.Message)
}
} else {
if testLogger.WarnCalled {
t.Error("did not expected warning to be logged,but it was.")
t.Errorf(testLogger.Message)
}
}
})
})
})

when("#SaveFile", func() {
Expand Down
6 changes: 5 additions & 1 deletion local/new.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,11 @@ func NewImage(repoName string, dockerClient DockerClient, ops ...imgutil.ImageOp
baseIdentifier = baseImage.identifier
store = baseImage.layerStore
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Base on my comment on the ImageOption, maybe we should change the constructor to:

func NewImage(repoName string, dockerClient DockerClient, logger Logger, ops ...imgutil.ImageOption) (*Image, error) {
   // create the Store with the Logger
}

Also in the NewStore constructor we can receive the Logger instance.

I know this is going to break, imgutil library consumers, but the Logger inside the ImageOption doesn't make sense for me, at least conceptually

store = NewStore(dockerClient)
if options.Logger != nil {
store = NewStore(dockerClient, WithStoreLogger(options.Logger))
} else {
store = NewStore(dockerClient)
}
}

cnbImage, err := imgutil.NewCNBImage(*options)
Expand Down
12 changes: 12 additions & 0 deletions local/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,15 @@ func WithMediaTypes(m imgutil.MediaTypes) func(*imgutil.ImageOptions) {
func WithPreviousImage(name string) func(*imgutil.ImageOptions) {
return imgutil.WithPreviousImage(name)
}

func WithLogger(logger imgutil.Logger) func(*imgutil.ImageOptions) {
return imgutil.WithLogger(logger)
}

type StoreOption func(*Store)

func WithStoreLogger(logger imgutil.Logger) StoreOption {
return func(s *Store) {
s.Logger = logger
}
}
14 changes: 12 additions & 2 deletions local/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
// optional
downloadOnce *sync.Once
onDiskLayersByDiffID map[v1.Hash]annotatedLayer
Logger imgutil.Logger
}

// DockerClient is subset of client.CommonAPIClient required by this package.
Expand All @@ -53,12 +54,18 @@
uncompressedSize int64
}

func NewStore(dockerClient DockerClient) *Store {
return &Store{
func NewStore(dockerClient DockerClient, ops ...StoreOption) *Store {
store := &Store{
dockerClient: dockerClient,
downloadOnce: &sync.Once{},
onDiskLayersByDiffID: make(map[v1.Hash]annotatedLayer),
Logger: nil,
}
for _, op := range ops {
op(store)
}

return store
}

// images
Expand Down Expand Up @@ -95,10 +102,13 @@
inspect, err = s.doSave(image, withName)
}
if !canOmitBaseLayers || err != nil {
if s.Logger != nil && !canOmitBaseLayers {
s.Logger.Warn("Containerd is enabled, which will make saving your work more expensive.")
}
if err = image.ensureLayers(); err != nil {
return "", err
}
inspect, err = s.doSave(image, withName)

Check warning on line 111 in local/store.go

View check run for this annotation

Codecov / codecov/patch

local/store.go#L106-L111

Added lines #L106 - L111 were not covered by tests
if err != nil {
saveErr := imgutil.SaveError{}
for _, n := range append([]string{withName}, withAdditionalNames...) {
Expand Down
12 changes: 12 additions & 0 deletions options.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ type ImageOptions struct {
BaseImageRepoName string
PreviousImageRepoName string
Config *v1.Config
Logger Logger
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I see the option carefully, I think we shouldn't expose the Logger as an ImageOption, because it is not an Image attribute.

I will remove it from here and move it to local.NewImage constructor.

CreatedAt time.Time
MediaTypes MediaTypes
Platform Platform
Expand All @@ -43,6 +44,10 @@ type RegistrySetting struct {
Insecure bool
}

type Logger interface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move the Logger interface to local or maybe to the root folder?

Warn(msg string)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@natalieparellano should we add a Warnf(fmt string, v ...interface{}) method too?

}

// FromBaseImage loads the provided image as the manifest, config, and layers for the working image.
// If the image is not found, it does nothing.
func FromBaseImage(name string) func(*ImageOptions) {
Expand Down Expand Up @@ -99,6 +104,13 @@ func WithPreviousImage(name string) func(*ImageOptions) {
}
}

// WithLogger if provided will check if contained is used.
func WithLogger(logger Logger) func(*ImageOptions) {
return func(o *ImageOptions) {
o.Logger = logger
}
}

type IndexOption func(options *IndexOptions) error

type IndexOptions struct {
Expand Down