fixed buG resulting from uniform extraction of image string from dock…

…er.Named struct

pkg/remoteimage/pull.go

@@ -15,7 +15,8 @@ import (
 
 type Puller interface {
 	Pull(context.Context) error
-	Image() string
+	ImageWithTag() string
+	ImageWithoutTag() string
 	ImageSize(context.Context) (int, error)
 }
 
@@ -34,7 +35,11 @@ type puller struct {
 	keyring  credentialprovider.DockerKeyring
 }
 
-func (p puller) Image() string {
+func (p puller) ImageWithTag() string {
+	return p.image.String()
+}
+
+func (p puller) ImageWithoutTag() string {
 	return p.image.Name()
 }
 
@@ -100,8 +105,8 @@ func (p puller) Pull(ctx context.Context) (err error) {
 			}
 		}
 	}()
-	repo := p.Image()
-	imageSpec := &cri.ImageSpec{Image: p.Image()}
+	repo := p.ImageWithoutTag()
+	imageSpec := &cri.ImageSpec{Image: p.ImageWithTag()}
 	creds, withCredentials := p.keyring.Lookup(repo)
 	// klog.V(2).Infof("remoteimage.Pull(): len(creds)=%d, withCreds=%t", len(creds), withCredentials)
 	if !withCredentials {

pkg/remoteimageasync/patterns_test.go

@@ -2,9 +2,12 @@ package remoteimageasync
 
 import (
 	"fmt"
+	"strings"
 	"testing"
 
+	"github.com/containerd/containerd/reference/docker"
 	"github.com/stretchr/testify/assert"
+	"github.com/warm-metal/container-image-csi-driver/pkg/remoteimage"
 )
 
 // demonstrates session channel structure's pass-by-reference is appropriate
@@ -72,3 +75,12 @@ func TestChannelClose(t *testing.T) {
 	assert.NotNil(t, err, "error should have been returned")
 	assert.Contains(t, err.Error(), "closed", "error should indicate channel closed")
 }
+
+func TestNamedImageExtraction(t *testing.T) {
+	parsed, err := docker.ParseDockerRef(nonExistentImage)
+	assert.Nil(t, err, "parsing image name should succeed")
+	puller := remoteimage.NewPuller(nil, parsed, nil)
+	assert.Equal(t, nonExistentImage, puller.ImageWithTag(), "extracted value should match exactly %v", puller)
+	repo := strings.Split(nonExistentImage, ":")[0]
+	assert.Equal(t, repo, puller.ImageWithoutTag(), "extracted value should match exactly %v", puller)
+}

pkg/remoteimageasync/puller.go

@@ -27,7 +27,7 @@ func RunPullerLoop(
 			}
 			go func() {
 				klog.V(2).Infof("%s.RunPullerLoop(): asked to pull image %s with timeout %v\n",
-					prefix, ses.Image(), ses.timeout)
+					prefix, ses.ImageWithTag(), ses.timeout)
 				ctxAsyncPullTimeoutOrShutdown, cancelDontCare := context.WithTimeout(ctx, ses.timeout) // combine session timeout and shut down signal into one
 				defer cancelDontCare()                                                                 // IF we exit, this no longer matters. calling to satisfy linter.
 				pullStart := time.Now()
@@ -41,13 +41,13 @@ func RunPullerLoop(
 					metrics.OperationErrorsCount.WithLabelValues("pull-async-shutdown").Inc()
 				case <-ctxAsyncPullTimeoutOrShutdown.Done(): // async pull timeout or shutdown
 					ses.isTimedOut = true
-					ses.err = fmt.Errorf("%s.RunPullerLoop(): async pull exceeded timeout of %v for image %s", prefix, ses.timeout, ses.Image())
+					ses.err = fmt.Errorf("%s.RunPullerLoop(): async pull exceeded timeout of %v for image %s", prefix, ses.timeout, ses.ImageWithTag())
 					klog.V(2).Infof(ses.err.Error())
 					metrics.OperationErrorsCount.WithLabelValues("pull-async-timeout").Inc()
 				default: // completion: success or error
 					ses.isTimedOut = false
 					ses.err = pullErr
-					klog.V(2).Infof("%s.RunPullerLoop(): pull completed in %v for image %s with error=%v\n", prefix, time.Since(pullStart), ses.Image(), ses.err)
+					klog.V(2).Infof("%s.RunPullerLoop(): pull completed in %v for image %s with error=%v\n", prefix, time.Since(pullStart), ses.ImageWithTag(), ses.err)
 					if ses.err != nil {
 						metrics.OperationErrorsCount.WithLabelValues("pull-async-error").Inc()
 					}

pkg/remoteimageasync/synchronizer.go

@@ -21,8 +21,8 @@ func StartAsyncPuller(ctx context.Context, sessionChanDepth int) AsyncPuller {
 	completedFunc := func(ses *PullSession) { // remove session from session map (since no longer active for continuation)
 		async.mutex.Lock()
 		defer async.mutex.Unlock()
-		klog.V(2).Infof("%s.StartAsyncPuller(): clearing session for %s", prefix, ses.Image())
-		delete(async.sessionMap, ses.Image()) // no-op if already deleted
+		klog.V(2).Infof("%s.StartAsyncPuller(): clearing session for %s", prefix, ses.ImageWithTag())
+		delete(async.sessionMap, ses.ImageWithTag()) // no-op if already deleted
 	}
 	RunPullerLoop(ctx, sessionChan, completedFunc)
 	klog.Infof("%s.StartAsyncPuller(): async puller is operational", prefix)
@@ -63,37 +63,37 @@ func (s synchronizer) StartPull(image string, puller remoteimage.Puller, asyncPu
 			if rec := recover(); rec != nil { // handle session write panic due to closed sessionChan
 				// override named return values
 				ses = nil
-				err = fmt.Errorf("%s.StartPull(): cannot create pull session for %s at this time, reason: %v", prefix, ses.Image(), rec)
+				err = fmt.Errorf("%s.StartPull(): cannot create pull session for %s at this time, reason: %v", prefix, ses.ImageWithTag(), rec)
 				klog.V(2).Info(err.Error())
 			}
 		}()
 		select {
 		case s.sessions <- ses: // start session, check for deadlock... possibility of panic but only during app shutdown where Puller has already ceased to operate, handle with defer/recover
-			klog.V(2).Infof("%s.StartPull(): new session created for %s with timeout %v", prefix, ses.Image(), ses.timeout)
+			klog.V(2).Infof("%s.StartPull(): new session created for %s with timeout %v", prefix, ses.ImageWithTag(), ses.timeout)
 			s.sessionMap[image] = ses // add session to map to allow continuation... only do this because was passed to puller via sessions channel
 			return ses, nil
 		default: // catch deadlock or throttling (they may look the same)
-			err := fmt.Errorf("%s.StartPull(): cannot create pull session for %s at this time, throttling or deadlock condition exists, retry if throttling", prefix, ses.Image())
+			err := fmt.Errorf("%s.StartPull(): cannot create pull session for %s at this time, throttling or deadlock condition exists, retry if throttling", prefix, ses.ImageWithTag())
 			klog.V(2).Info(err.Error())
 			return nil, err
 		}
 	} else {
-		klog.V(2).Infof("%s.StartPull(): found open session for %s", prefix, ses.Image())
+		klog.V(2).Infof("%s.StartPull(): found open session for %s", prefix, ses.ImageWithTag())
 		// return session and unlock
 		return ses, nil
 	}
 }
 
 func (s synchronizer) WaitForPull(session *PullSession, callerTimeout context.Context) error {
-	klog.V(2).Infof("%s.WaitForPull(): starting to wait for image %s", prefix, session.Image())
-	defer klog.V(2).Infof("%s.WaitForPull(): exiting wait for image %s", prefix, session.Image())
+	klog.V(2).Infof("%s.WaitForPull(): starting to wait for image %s", prefix, session.ImageWithTag())
+	defer klog.V(2).Infof("%s.WaitForPull(): exiting wait for image %s", prefix, session.ImageWithTag())
 	select {
 	case <-session.done: // success or error (including session timeout and shutting down)
-		klog.V(2).Infof("%s.WaitForPull(): session completed with success or error for image %s, error=%v", prefix, session.Image(), session.err)
+		klog.V(2).Infof("%s.WaitForPull(): session completed with success or error for image %s, error=%v", prefix, session.ImageWithTag(), session.err)
 		return session.err
 	case <-callerTimeout.Done(): // caller timeout
 		err := fmt.Errorf("%s.WaitForPull(): this wait for image %s has timed out due to caller context cancellation, pull likely continues in the background",
-			prefix, session.Image())
+			prefix, session.ImageWithTag())
 		klog.V(2).Info(err.Error())
 		return err
 	}

pkg/remoteimageasync/synchronizer_test.go

@@ -212,10 +212,14 @@ func (p pullerMock) Pull(ctx context.Context) (err error) {
 	}
 }
 
-func (p pullerMock) Image() string {
+func (p pullerMock) ImageWithTag() string {
 	return p.image
 }
 
+func (p pullerMock) ImageWithoutTag() string {
+	panic("Not implemented")
+}
+
 func (p pullerMock) ImageSize(ctx context.Context) (int, error) {
 	if p.size < 0 {
 		return 0, fmt.Errorf("error occurred when checking image size")

pkg/remoteimageasync/types.go

@@ -18,8 +18,8 @@ type PullSession struct {
 	err        error
 }
 
-func (p PullSession) Image() string {
-	return p.puller.Image()
+func (p PullSession) ImageWithTag() string {
+	return p.puller.ImageWithTag()
 }
 
 type synchronizer struct {