removeImageRegistry should be able to remove the registry during bootstrap

Author: dmage

pkg/operator/controller.go

@@ -278,7 +278,7 @@ func (c *Controller) sync() error {
 	}
 	c.syncStatus(cr, deploy, routes, applyError)
 
-	metadataChanged := strategy.Metadata(&prevCR.ObjectMeta, &cr.ObjectMeta)
+	metadataChanged := strategy.Metadata(prevCR.ObjectMeta.DeepCopy(), &cr.ObjectMeta)
 	specChanged := !reflect.DeepEqual(prevCR.Spec, cr.Spec)
 	if metadataChanged || specChanged {
 		difference, err := object.DiffString(prevCR, cr)
@@ -299,7 +299,12 @@ func (c *Controller) sync() error {
 				updatedCR.ObjectMeta = cr.ObjectMeta
 			}
 			if specChanged {
+				// FIXME: Here be dragons. The operator can
+				// accidentally lose user-provided
+				// configuration.
+				managementState := updatedCR.Spec.ManagementState
 				updatedCR.Spec = cr.Spec
+				updatedCR.Spec.ManagementState = managementState
 			}
 
 			updatedCR, err = c.clients.RegOp.ImageregistryV1().Configs().Update(
@@ -313,6 +318,10 @@ func (c *Controller) sync() error {
 		// If we updated the Status field too, we'll make one more call and we
 		// want it to succeed.
 		cr.ResourceVersion = updatedCR.ResourceVersion
+
+		// Update prevCR to make diff accurate.
+		prevCR.ObjectMeta = updatedCR.ObjectMeta
+		prevCR.Spec = updatedCR.Spec
 	}
 
 	cr.Status.ObservedGeneration = cr.Generation

test/e2e/aws_test.go

@@ -760,12 +760,6 @@ func TestAWSFinalizerDeleteS3Bucket(t *testing.T) {
 	if exists {
 		t.Errorf("s3 bucket should have been deleted, but it wasn't: %s", err)
 	}
-
-	// Once the config object is deleted, the operator will create a new one.
-	// The testing framework does not expect creation of a new config object
-	// when it is in the teardown stage, so let's wait until the operator
-	// stabilizes again.
-	framework.WaitUntilImageRegistryIsAvailable(te)
 }
 
 // createAWSConfigFile creates an AWS credentials config based on the contents of the Secret

test/framework/imageregistry.go

@@ -91,35 +91,82 @@ func (c ImageRegistryConditions) String() string {
 
 func removeImageRegistry(te TestEnv) {
 	te.Logf("uninstalling the image registry...")
+
+	operatorDeployment, err := te.Client().Deployments(OperatorDeploymentNamespace).Get(
+		context.Background(), OperatorDeploymentName, metav1.GetOptions{},
+	)
+	if err != nil {
+		te.Fatalf("unable to get the operator deployment: %s", err)
+	}
+
+	if !isDeploymentRolledOut(operatorDeployment) {
+		te.Errorf("unexepected state: the operator is not rolled out before removing the image registry")
+	}
+
+	if operatorDeployment.Spec.Replicas != nil && *operatorDeployment.Spec.Replicas == 0 {
+		config, err := te.Client().Configs().Get(
+			context.Background(), defaults.ImageRegistryResourceName, metav1.GetOptions{},
+		)
+		if errors.IsNotFound(err) {
+			return
+		} else if err != nil {
+			te.Fatalf("unable to get the image registry config: %s", err)
+		}
+		conds := GetImageRegistryConditions(config)
+		if !conds.Removed.IsTrue() {
+			te.Fatalf("unable to uninstall the image registry: the operator is shutted down, but the image registry is not removed: %s", config.Spec.ManagementState, conds)
+		}
+		return
+	}
+
+	err = wait.PollImmediate(2*time.Second, 30*time.Second, func() (stop bool, err error) {
+		cr, err := te.Client().Configs().Get(
+			context.Background(), defaults.ImageRegistryResourceName, metav1.GetOptions{},
+		)
+		if err != nil {
+			te.Logf("the image registry config is not found: %s", err)
+			return false, nil
+		}
+		if cr.DeletionTimestamp != nil {
+			te.Logf("the image registry config is being deleted: %s", cr.DeletionTimestamp)
+			return false, nil
+		}
+		return true, nil
+	})
+	if err != nil {
+		te.Fatalf("failed to wait until the operator creates the config object: %s", err)
+	}
+
 	if _, err := te.Client().Configs().Patch(
 		context.Background(),
 		defaults.ImageRegistryResourceName,
 		types.MergePatchType,
 		[]byte(`{"spec": {"managementState": "Removed"}}`),
 		metav1.PatchOptions{},
 	); err != nil {
-		if errors.IsNotFound(err) {
-			// That's not exactly what we are asked for. And few seconds later
-			// the operator may bootstrap it. However, if the operator is
-			// disabled, it means the registry is not installed and we're
-			// already in the desired state.
-			return
-		}
 		te.Fatalf("unable to uninstall the image registry: %s", err)
 	}
 
 	var cr *imageregistryapiv1.Config
-	err := wait.Poll(5*time.Second, AsyncOperationTimeout, func() (stop bool, err error) {
+	err = wait.Poll(5*time.Second, AsyncOperationTimeout, func() (stop bool, err error) {
 		cr, err = te.Client().Configs().Get(
 			context.Background(), defaults.ImageRegistryResourceName, metav1.GetOptions{},
 		)
 		if errors.IsNotFound(err) {
+			te.Logf("waiting for the registry to be removed: the config object does not exist?!")
 			cr = nil
 			return true, nil
 		} else if err != nil {
+			te.Logf("waiting for the registry to be removed: %s", err)
 			return false, err
 		}
 
+		if cr.Spec.ManagementState != "Removed" {
+			DumpYAML(te, "unexpected management state in the config object", cr)
+			DumpOperatorLogs(te)
+			te.Fatalf("unexpected management state: got %s, want Removed", cr.Spec.ManagementState)
+		}
+
 		conds := GetImageRegistryConditions(cr)
 		te.Logf("waiting for the registry to be removed: %s", conds)
 		return conds.Progressing.IsFalse() && conds.Removed.IsTrue(), nil