fix clear out of specific tag errors (stream conditions in fact are e…

…rased in tag clean)
openshift · Nov 16, 2020 · 2c44279 · 2c44279
1 parent 49ea480
commit 2c44279
Show file tree

Hide file tree

Showing 4 changed files with 133 additions and 33 deletions.
diff --git a/pkg/stub/config.go b/pkg/stub/config.go
@@ -221,16 +221,13 @@ func (h *Handler) buildFileMaps(cfg *v1.Config, forceRebuild bool) error {
 		cm = &corev1.ConfigMap{}
 		cm.Name = util.IST2ImageMap
 		cm.Namespace = v1.OperatorNamespace
-		if cm.Annotations == nil {
-			cm.Annotations = map[string]string{}
-		}
+		cm.Annotations = map[string]string{}
 		cm.Annotations[v1.SamplesVersionAnnotation] = h.version
 		cm.Data = map[string]string{}
 		for key, value := range h.imagestreatagToImage {
 			cm.Data[key] = value
 		}
-		_, err = h.configmapclientwrapper.Create(cm)
-		if err != nil {
+		if _, err = h.configmapclientwrapper.Create(cm); err != nil {
 			return err
 		}
 	}

diff --git a/pkg/stub/configmap.go b/pkg/stub/configmap.go
@@ -166,35 +166,34 @@ func (h *Handler) storeImageStreamTagError(isName, tagName, message string) erro
 	return err
 }
 
-func (h *Handler) clearImageStreamTagError(isName, tagName string) error {
+func (h *Handler) clearImageStreamTagError(isName string, tags []string) error {
 	cm, err := h.configmapclientwrapper.Get(isName)
 	if err != nil {
 		if !kerrors.IsNotFound(err) {
 			logrus.Warningf("unexpected error on get of configmap %s: %s", isName, err.Error())
 			return err
 		}
-		cm := &corev1.ConfigMap{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      isName,
-				Namespace: v1.OperatorNamespace,
-			},
-			Data: map[string]string{},
-		}
-		cm, err = h.configmapclientwrapper.Create(cm)
-		if err != nil && !kerrors.IsAlreadyExists(err) {
-			return err
-		}
+		logrus.Printf("clearImageStreamTagError: stream %s already deleted so no worries on clearing tags", isName)
+		return nil
 	}
 	if cm.Data == nil {
 		return nil
 	}
-	_, hasTag := cm.Data[tagName]
-	if !hasTag {
-		return nil
+	hasTag := false
+	for _, tagName := range tags {
+		_, ok := cm.Data[tagName]
+		if !ok {
+			continue
+		}
+		hasTag = true
+		logrus.Printf("clearing error messages from configmap for stream %s and tag %s", isName, tagName)
+		delete(cm.Data, tagName)
+
 	}
-	delete(cm.Data, tagName)
 
-	_, err = h.configmapclientwrapper.Update(cm)
+	if hasTag {
+		_, err = h.configmapclientwrapper.Update(cm)
+	}
 	return err
 
 }
diff --git a/pkg/stub/handler_test.go b/pkg/stub/handler_test.go
@@ -1117,6 +1117,92 @@ func TestImageStreamImportError(t *testing.T) {
 	}
 }
 
+func TestImageStreamTagImportErrorRecovery(t *testing.T) {
+	two := int64(2)
+	one := int64(1)
+	stream := &imagev1.ImageStream{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "foo",
+			Annotations: map[string]string{
+				v1.SamplesVersionAnnotation: TestVersion,
+			},
+		},
+		Spec: imagev1.ImageStreamSpec{
+			Tags: []imagev1.TagReference{
+				{
+					Name:       "1",
+					Generation: &two,
+				},
+				{
+					Name:       "2",
+					Generation: &one,
+				},
+			},
+		},
+		Status: imagev1.ImageStreamStatus{
+			Tags: []imagev1.NamedTagEventList{
+				{
+					Tag: "1",
+					Items: []imagev1.TagEvent{
+						{
+							Generation: two,
+						},
+					},
+					Conditions: []imagev1.TagEventCondition{
+						{
+							Status:     corev1.ConditionFalse,
+							Generation: two,
+						},
+					},
+				},
+				{
+					Tag: "2",
+					Items: []imagev1.TagEvent{
+						{
+							Generation: two,
+						},
+					},
+				},
+			},
+		},
+	}
+	h, cfg, _ := setup()
+	mimic(&h, x86ContentRootDir)
+	dir := h.GetBaseDir(v1.X86Architecture, cfg)
+	files, _ := h.Filefinder.List(dir)
+	h.processFiles(dir, files, cfg)
+	importError := util.Condition(cfg, v1.ImportImageErrorsExist)
+	importError.Status = corev1.ConditionTrue
+	cm := &corev1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "foo",
+		},
+		Data: map[string]string{
+			"1": "could not import",
+			"2": "could not import",
+		},
+	}
+	fakecmclient := h.configmapclientwrapper.(*fakeConfigMapClientWrapper)
+	fakecmclient.configMaps["foo"] = cm
+	util.ConditionUpdate(cfg, importError)
+	err := h.processImageStreamWatchEvent(stream, false)
+	if err != nil {
+		t.Fatalf("processImageStreamWatchEvent error %#v", err)
+	}
+	cm, stillHasCM := fakecmclient.configMaps["foo"]
+	if !stillHasCM {
+		t.Fatalf("partially clean imagestream did not keep configmap")
+	}
+	_, stillHasCleanTag := cm.Data["2"]
+	if stillHasCleanTag {
+		t.Fatalf("partially clean imagestream still has clean tag in configmap")
+	}
+	_, stillHasBrokenTag := cm.Data["1"]
+	if !stillHasBrokenTag {
+		t.Fatalf("partially clean imagestream does not have tag that is still broke in configmap")
+	}
+}
+
 func TestImageStreamImportErrorRecovery(t *testing.T) {
 	two := int64(2)
 	one := int64(1)

diff --git a/pkg/stub/imagestreams.go b/pkg/stub/imagestreams.go
@@ -315,6 +315,7 @@ func (h *Handler) processImportStatus(is *imagev1.ImageStream, cfg *v1.Config) (
 			retryIfNeeded = lastRetryTime.Time.Before(tenMinutesAgo)
 		}
 
+		tagsToPotentiallClearFromConfigMap := []string{}
 		for _, statusTag := range is.Status.Tags {
 			// if an error occurred with the latest generation, let's give up as we are no longer "in progress"
 			// in that case as well, but mark the import failure
@@ -336,8 +337,6 @@ func (h *Handler) processImportStatus(is *imagev1.ImageStream, cfg *v1.Config) (
 				if mostRecentErrorGeneration > 0 && mostRecentErrorGeneration >= latestGeneration {
 					logrus.Warningf("Image import for imagestream %s tag %s generation %v failed with detailed message %s", is.Name, statusTag.Tag, mostRecentErrorGeneration, message)
 					anyErrors = true
-					// update imagestream error message for this tag
-					err = h.storeImageStreamTagError(is.Name, statusTag.Tag, message)
 
 					// if a first time failure, or need to retry
 					if !h.imageStreamHasErrors(is.Name) ||
@@ -347,27 +346,46 @@ func (h *Handler) processImportStatus(is *imagev1.ImageStream, cfg *v1.Config) (
 						imgImport, err := importTag(is, statusTag.Tag)
 						if err != nil {
 							logrus.Warningf("attempted to define and imagestreamimport for imagestream/tag %s/%s but got err %v; simply moving on", is.Name, statusTag.Tag, err)
-							break
 						}
 						if imgImport == nil {
-							break
+							logrus.Warningf("attempted to define an imagestreamimport for imagestream/tag %s/%s bug got a nil image import reference", is.Name, statusTag.Tag)
 						}
-						imgImport, err = h.imageclientwrapper.ImageStreamImports("openshift").Create(context.TODO(), imgImport, kapis.CreateOptions{})
-						if err != nil {
-							logrus.Warningf("attempted to initiate an imagestreamimport retry for imagestream/tag %s/%s but got err %v; simply moving on", is.Name, statusTag.Tag, err)
-							break
+						if imgImport != nil && err == nil {
+							imgImport, err = h.imageclientwrapper.ImageStreamImports("openshift").Create(context.TODO(), imgImport, kapis.CreateOptions{})
+							if err != nil {
+								logrus.Warningf("attempted to initiate an imagestreamimport retry for imagestream/tag %s/%s but got err %v; simply moving on", is.Name, statusTag.Tag, err)
+							}
+							if err == nil {
+								metrics.ImageStreamImportRetry(is.Name)
+								logrus.Printf("initiated an imagestreamimport retry for imagestream/tag %s/%s", is.Name, statusTag.Tag)
+							}
+
 						}
-						metrics.ImageStreamImportRetry(is.Name)
-						logrus.Printf("initiated an imagestreamimport retry for imagestream/tag %s/%s", is.Name, statusTag.Tag)
 
 					}
 
+					// update imagestream error message for this tag
+					err = h.storeImageStreamTagError(is.Name, statusTag.Tag, message)
+					if err != nil {
+						return cfg, err
+					}
+
 				} else {
-					h.clearImageStreamTagError(is.Name, statusTag.Tag)
+					// in case the currently observed behavior of imagestreams changes and the conditions array is not
+					// cleared out when a tag is healthy, clear the tag here as well
+					// but at present time, this path will not be hit
+					tagsToPotentiallClearFromConfigMap = append(tagsToPotentiallClearFromConfigMap, statusTag.Tag)
 				}
+			} else {
+				// lack of conditions means there are no errors; conditions seem to now be cleared if errors resolved
+				tagsToPotentiallClearFromConfigMap = append(tagsToPotentiallClearFromConfigMap, statusTag.Tag)
 			}
 		}
 
+		if len(tagsToPotentiallClearFromConfigMap) > 0 {
+			err = h.clearImageStreamTagError(is.Name, tagsToPotentiallClearFromConfigMap)
+		}
+
 	} else {
 		logrus.Debugf("no error/progress checks cause stream %s is skipped", is.Name)
 		// but if skipped, clear out any errors, since we do not care about errors for skipped