Merge pull request #15899 from soltysh/pruning_retries

Automatic merge from submit-queue

Retry image stream updates when pruning images

@mfojtik @brenton ptal, this is a bug fix so I guess we can merge it at any point

@miminar fyi

pkg/image/prune/prune.go

@@ -20,6 +20,7 @@ import (
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/sets"
 	kapi "k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/client/retry"
 
 	"github.com/openshift/origin/pkg/api/graph"
 	kubegraph "github.com/openshift/origin/pkg/api/kubegraph/nodes"
@@ -71,9 +72,11 @@ type ImageDeleter interface {
 
 // ImageStreamDeleter knows how to remove an image reference from an image stream.
 type ImageStreamDeleter interface {
-	// DeleteImageStream removes all references to the image from the image
+	// GetImageStream returns a fresh copy of an image stream.
+	GetImageStream(stream *imageapi.ImageStream) (*imageapi.ImageStream, error)
+	// UpdateImageStream removes all references to the image from the image
 	// stream's status.tags. The updated image stream is returned.
-	DeleteImageStream(stream *imageapi.ImageStream, image *imageapi.Image, updatedTags []string) (*imageapi.ImageStream, error)
+	UpdateImageStream(stream *imageapi.ImageStream, image *imageapi.Image, updatedTags []string) (*imageapi.ImageStream, error)
 }
 
 // BlobDeleter knows how to delete a blob from the Docker registry.
@@ -671,7 +674,7 @@ func calculatePrunableImageComponents(g graph.Graph) []*imagegraph.ImageComponen
 }
 
 // pruneStreams removes references from all image streams' status.tags entries
-// to prunable images, invoking streamPruner.DeleteImageStream for each updated
+// to prunable images, invoking streamPruner.UpdateImageStream for each updated
 // stream.
 func pruneStreams(g graph.Graph, imageNodes []*imagegraph.ImageNode, streamPruner ImageStreamDeleter) []error {
 	errs := []error{}
@@ -684,43 +687,49 @@ func pruneStreams(g graph.Graph, imageNodes []*imagegraph.ImageNode, streamPrune
 				continue
 			}
 
-			stream := streamNode.ImageStream
-			updatedTags := sets.NewString()
+			if err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+				stream, err := streamPruner.GetImageStream(streamNode.ImageStream)
+				if err != nil {
+					return err
+				}
+				updatedTags := sets.NewString()
 
-			glog.V(4).Infof("Checking if ImageStream %s has references to image %s in status.tags", getName(stream), imageNode.Image.Name)
+				glog.V(4).Infof("Checking if ImageStream %s has references to image %s in status.tags", getName(stream), imageNode.Image.Name)
 
-			for tag, history := range stream.Status.Tags {
-				glog.V(4).Infof("Checking tag %q", tag)
+				for tag, history := range stream.Status.Tags {
+					glog.V(4).Infof("Checking tag %q", tag)
 
-				newHistory := imageapi.TagEventList{}
+					newHistory := imageapi.TagEventList{}
 
-				for i, tagEvent := range history.Items {
-					glog.V(4).Infof("Checking tag event %d with image %q", i, tagEvent.Image)
+					for i, tagEvent := range history.Items {
+						glog.V(4).Infof("Checking tag event %d with image %q", i, tagEvent.Image)
 
-					if tagEvent.Image != imageNode.Image.Name {
-						glog.V(4).Infof("Tag event doesn't match deleted image - keeping")
-						newHistory.Items = append(newHistory.Items, tagEvent)
+						if tagEvent.Image != imageNode.Image.Name {
+							glog.V(4).Infof("Tag event doesn't match deleted image - keeping")
+							newHistory.Items = append(newHistory.Items, tagEvent)
+						} else {
+							glog.V(4).Infof("Tag event matches deleted image - removing reference")
+							updatedTags.Insert(tag)
+						}
+					}
+					if len(newHistory.Items) == 0 {
+						glog.V(4).Infof("Removing tag %q from status.tags of ImageStream %s", tag, getName(stream))
+						delete(stream.Status.Tags, tag)
 					} else {
-						glog.V(4).Infof("Tag event matches deleted image - removing reference")
-						updatedTags.Insert(tag)
+						stream.Status.Tags[tag] = newHistory
 					}
 				}
-				if len(newHistory.Items) == 0 {
-					glog.V(4).Infof("Removing tag %q from status.tags of ImageStream %s", tag, getName(stream))
-					delete(stream.Status.Tags, tag)
-				} else {
-					stream.Status.Tags[tag] = newHistory
-				}
-			}
 
-			updatedStream, err := streamPruner.DeleteImageStream(stream, imageNode.Image, updatedTags.List())
-			if err != nil {
+				updatedStream, err := streamPruner.UpdateImageStream(stream, imageNode.Image, updatedTags.List())
+				if err == nil {
+					streamNode.ImageStream = updatedStream
+				}
+				return err
+			}); err != nil {
 				errs = append(errs, fmt.Errorf("error removing image %s from stream %s: %v",
-					imageNode.Image.Name, getName(stream), err))
+					imageNode.Image.Name, getName(streamNode.ImageStream), err))
 				continue
 			}
-
-			streamNode.ImageStream = updatedStream
 		}
 	}
 	glog.V(4).Infof("Done removing pruned image references from streams")
@@ -922,7 +931,11 @@ func NewImageStreamDeleter(streams client.ImageStreamsNamespacer) ImageStreamDel
 	}
 }
 
-func (p *imageStreamDeleter) DeleteImageStream(stream *imageapi.ImageStream, image *imageapi.Image, updatedTags []string) (*imageapi.ImageStream, error) {
+func (p *imageStreamDeleter) GetImageStream(stream *imageapi.ImageStream) (*imageapi.ImageStream, error) {
+	return p.streams.ImageStreams(stream.Namespace).Get(stream.Name, metav1.GetOptions{})
+}
+
+func (p *imageStreamDeleter) UpdateImageStream(stream *imageapi.ImageStream, image *imageapi.Image, updatedTags []string) (*imageapi.ImageStream, error) {
 	glog.V(4).Infof("Updating ImageStream %s", getName(stream))
 	is, err := p.streams.ImageStreams(stream.Namespace).UpdateStatus(stream)
 	if err == nil {

pkg/image/prune/prune_test.go

@@ -365,7 +365,11 @@ type fakeImageStreamDeleter struct {
 
 var _ ImageStreamDeleter = &fakeImageStreamDeleter{}
 
-func (p *fakeImageStreamDeleter) DeleteImageStream(stream *imageapi.ImageStream, image *imageapi.Image, updatedTags []string) (*imageapi.ImageStream, error) {
+func (p *fakeImageStreamDeleter) GetImageStream(stream *imageapi.ImageStream) (*imageapi.ImageStream, error) {
+	return stream, p.err
+}
+
+func (p *fakeImageStreamDeleter) UpdateImageStream(stream *imageapi.ImageStream, image *imageapi.Image, updatedTags []string) (*imageapi.ImageStream, error) {
 	p.invocations.Insert(fmt.Sprintf("%s/%s|%s", stream.Namespace, stream.Name, image.Name))
 	return stream, p.err
 }

pkg/oc/admin/prune/images.go

@@ -58,9 +58,9 @@ var (
 		Insecure connection is allowed in the following cases unless certificate-authority is
 		specified:
 
-		 1. --force-insecure is given  
-		 2. provided registry-url is prefixed with http://  
-		 3. registry url is a private or link-local address  
+		 1. --force-insecure is given
+		 2. provided registry-url is prefixed with http://
+		 3. registry url is a private or link-local address
 		 4. user's config allows for insecure connection (the user logged in to the cluster with
 			--insecure-skip-tls-verify or allowed for insecure connection)`)
 
@@ -361,7 +361,11 @@ type describingImageStreamDeleter struct {
 
 var _ prune.ImageStreamDeleter = &describingImageStreamDeleter{}
 
-func (p *describingImageStreamDeleter) DeleteImageStream(stream *imageapi.ImageStream, image *imageapi.Image, updatedTags []string) (*imageapi.ImageStream, error) {
+func (p *describingImageStreamDeleter) GetImageStream(stream *imageapi.ImageStream) (*imageapi.ImageStream, error) {
+	return stream, nil
+}
+
+func (p *describingImageStreamDeleter) UpdateImageStream(stream *imageapi.ImageStream, image *imageapi.Image, updatedTags []string) (*imageapi.ImageStream, error) {
 	if !p.headerPrinted {
 		p.headerPrinted = true
 		fmt.Fprintln(p.w, "Deleting references from image streams to images ...")
@@ -374,7 +378,7 @@ func (p *describingImageStreamDeleter) DeleteImageStream(stream *imageapi.ImageS
 		return stream, nil
 	}
 
-	updatedStream, err := p.delegate.DeleteImageStream(stream, image, updatedTags)
+	updatedStream, err := p.delegate.UpdateImageStream(stream, image, updatedTags)
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "error updating image stream %s/%s to remove references to image %s: %v\n", stream.Namespace, stream.Name, image.Name, err)
 	}