Merge pull request openshift#125 from gabemontero/improve-version-status

Improve version status

pkg/apis/samples/v1/types.go

@@ -73,9 +73,9 @@ type ConfigSpec struct {
 	ManagementState operatorv1.ManagementState `json:"managementState,omitempty" protobuf:"bytes,1,opt,name=managementState"`
 
 	// SamplesRegistry allows for the specification of which registry is accessed
-	// by the ImageStreams for their image content.  Defaults depend on the InstallType.
-	// An InstallType of 'rhel' defaults to registry.redhat.io, and an InstallType of
-	// 'centos' defaults to docker.io.
+	// by the ImageStreams for their image content.  Defaults on the content in https://github.com/openshift/library
+	// that are pulled into this github repository, but based on our pulling only ocp content it typically
+	// defaults to registry.redhat.io.
 	SamplesRegistry string `json:"samplesRegistry,omitempty" protobuf:"bytes,2,opt,name=samplesRegistry"`
 
 	// Architectures determine which hardware architecture(s) to install, where x86_64 and ppc64le are the
@@ -106,9 +106,9 @@ type ConfigStatus struct {
 	Conditions []ConfigCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,2,rep,name=conditions"`
 
 	// SamplesRegistry allows for the specification of which registry is accessed
-	// by the ImageStreams for their image content.  Defaults depend on the InstallType.
-	// An InstallType of 'rhel' defaults to registry.redhat.io, and an InstallType of
-	// 'centos' defaults to docker.io.
+	// by the ImageStreams for their image content.  Defaults on the content in https://github.com/openshift/library
+	// that are pulled into this github repository, but based on our pulling only ocp content it typically
+	// defaults to registry.redhat.io.
 	SamplesRegistry string `json:"samplesRegistry,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,3,rep,name=samplesRegistry"`
 
 	// Architectures determine which hardware architecture(s) to install, where x86_64 and ppc64le are the
@@ -174,6 +174,9 @@ const (
 	// change cycle as complete in both ClusterOperator and Config; retry on import will
 	// occur by the next relist interval if it was an intermittent issue;
 	ImportImageErrorsExist ConfigConditionType = "ImportImageErrorsExist"
+	// numConfigConditionType is a helper constant that captures the number possible conditions
+	// defined above in this const block
+	numconfigConditionType = 7
 )
 
 // ConfigCondition captures various conditions of the Config
@@ -349,8 +352,9 @@ func (s *Config) ClusterOperatorStatusAvailableCondition() (configv1.ConditionSt
 // 2) the first string is the succinct text to apply to the Progressing condition on failure
 // 3) the second string is the fully detailed text to apply the the Failing condition
 func (s *Config) ClusterOperatorStatusFailingCondition() (configv1.ConditionStatus, string, string) {
-	if len(s.Status.Conditions) == 0 {
-		// first event, have not processed default config yet
+	// do not start checking for bad config and needed cred until we've iterated through
+	// the credential / config processing to actually processed a config
+	if len(s.Status.Conditions) < numconfigConditionType {
 		return configv1.ConditionFalse, "", ""
 	}
 	// the ordering here is not random; an invalid config will be caught first;

pkg/stub/config.go

@@ -274,11 +274,6 @@ func (h *Handler) ProcessManagementField(cfg *v1.Config) (bool, bool, error) {
 			condition.LastUpdateTime = now
 			condition.Status = corev1.ConditionFalse
 			cfg.ConditionUpdate(condition)
-			cred := cfg.Condition(v1.ImportCredentialsExist)
-			cred.LastTransitionTime = now
-			cred.LastUpdateTime = now
-			cred.Status = corev1.ConditionFalse
-			cfg.ConditionUpdate(cred)
 			cfg.Status.ManagementState = operatorsv1api.Removed
 			cfg.Status.Version = ""
 			h.ClearStatusConfigForRemoved(cfg)

pkg/stub/handler.go

@@ -384,16 +384,8 @@ func (h *Handler) CleanUpOpenshiftNamespaceOnDelete(cfg *v1.Config) error {
 		}
 	}
 
-	if cfg.ConditionTrue(v1.ImportCredentialsExist) {
-		logrus.Println("Operator is deleting the credential in the openshift namespace that was previously created.")
-		logrus.Println("If you are Removing content as part of switching from 'centos' to 'rhel', the credential will be recreated in the openshift namespace when you move back to 'Managed' state.")
-
-	}
-	err = h.secretclientwrapper.Delete("openshift", v1.SamplesRegistryCredentials, &metav1.DeleteOptions{})
-	if err != nil && !kerrors.IsNotFound(err) {
-		logrus.Warnf("Problem deleting openshift secret %s on Config delete: %#v", v1.SamplesRegistryCredentials, err)
-		return err
-	}
+	// FYI we no longer delete the credential because the payload imagestreams like cli, must-gather that
+	// this operator initially installs via its manifest, but does not manage, needs the pull image secret
 
 	return nil
 }

pkg/stub/handler_test.go

@@ -259,20 +259,21 @@ func TestProcessed(t *testing.T) {
 	tkeys := getTKeys()
 
 	mimic(&h, x86OCPContentRootDir)
+	processCred(&h, cfg, t)
 
 	err := h.Handle(event)
 	validate(true, err, "", cfg,
 		conditions,
-		[]corev1.ConditionStatus{corev1.ConditionFalse, corev1.ConditionFalse, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionFalse, corev1.ConditionFalse}, t)
+		[]corev1.ConditionStatus{corev1.ConditionFalse, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionFalse, corev1.ConditionFalse}, t)
 
 	err = h.Handle(event)
 	validate(true, err, "", cfg,
 		conditions,
-		[]corev1.ConditionStatus{corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionFalse, corev1.ConditionFalse}, t)
+		[]corev1.ConditionStatus{corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionFalse, corev1.ConditionFalse}, t)
 	err = h.Handle(event)
 	validate(true, err, "", cfg,
 		conditions,
-		[]corev1.ConditionStatus{corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionFalse, corev1.ConditionFalse}, t)
+		[]corev1.ConditionStatus{corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionFalse, corev1.ConditionFalse}, t)
 
 	fakeisclient := h.imageclientwrapper.(*fakeImageStreamClientWrapper)
 	for _, key := range iskeys {
@@ -313,7 +314,7 @@ func TestProcessed(t *testing.T) {
 	err = h.Handle(event)
 	validate(true, err, "", cfg,
 		conditions,
-		[]corev1.ConditionStatus{corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionFalse, corev1.ConditionFalse}, t)
+		[]corev1.ConditionStatus{corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionFalse, corev1.ConditionFalse}, t)
 	is, _ = fakeisclient.Get("", "foo", metav1.GetOptions{})
 	if is == nil || !strings.HasPrefix(is.Spec.DockerImageRepository, cfg.Spec.SamplesRegistry) {
 		t.Fatalf("stream repo not updated %#v, %#v", is, h)
@@ -507,20 +508,16 @@ func TestCreateDeleteSecretAfterCR(t *testing.T) {
 
 	h.secretRetryCount = 3 // bypass retry on CR update race
 	cfg.Spec.ManagementState = operatorsv1api.Removed
-	statuses[1] = corev1.ConditionFalse
+	statuses[1] = corev1.ConditionTrue
 	err = h.Handle(event)
-	// import cred should be false if from removed, since we don't recreate
+	// import cred should be true if from removed, since we don't delete on removed
 	validate(true, err, "", cfg, conditions, statuses, t)
 
 	cfg.Spec.ManagementState = operatorsv1api.Managed
 	h.secretRetryCount = 3
 	err = h.Handle(event)
-	// even though we recreate the secret, we wait for the event to come in
-	// to set import cred to true
-	validate(true, err, "", cfg, conditions, statuses, t)
-	// mimic event from recreate that should have occurred
-	processCred(&h, cfg, t)
-	statuses[1] = corev1.ConditionTrue
+	statuses[3] = corev1.ConditionTrue
+	// with secret still present, we should start import images
 	validate(true, err, "", cfg, conditions, statuses, t)
 
 }
@@ -529,6 +526,8 @@ func setup() (Handler, *v1.Config, v1.Event) {
 	h := NewTestHandler()
 	cfg, _ := h.CreateDefaultResourceIfNeeded(nil)
 	cfg = h.initConditions(cfg)
+	fakesecretclient := h.secretclientwrapper.(*fakeSecretClientWrapper)
+	fakesecretclient.err = kerrors.NewNotFound(schema.GroupResource{}, v1.SamplesRegistryCredentials)
 	h.crdwrapper.(*fakeCRDWrapper).cfg = cfg
 	cache.ClearUpsertsCache()
 	return h, cfg, v1.Event{Object: cfg}
@@ -538,6 +537,7 @@ func processCred(h *Handler, cfg *v1.Config, t *testing.T) {
 	if !cfg.ConditionFalse(v1.ImportCredentialsExist) {
 		t.Fatalf("import cred exists unexpectedly true: %#v", cfg)
 	}
+	h.secretclientwrapper.(*fakeSecretClientWrapper).err = nil
 	secret := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      v1.SamplesRegistryCredentials,

pkg/stub/secrets.go

@@ -104,7 +104,10 @@ func (h *Handler) WaitingForCredential(cfg *v1.Config) (bool, bool) {
 	// if trying to do rhel to the default registry.redhat.io registry requires the secret
 	// be in place since registry.redhat.io requires auth to pull; since it is not ready
 	// log error state
-	if cfg.ClusterNeedsCreds() {
+	// we check for actual existence vs. condition because in delete/recreate scenario, the condition can't
+	// be added out of the gate
+	_, err := h.secretclientwrapper.Get("openshift", v1.SamplesRegistryCredentials)
+	if err != nil {
 		cred := cfg.Condition(v1.ImportCredentialsExist)
 		// - if import cred is false, and the message is empty, that means we have NOT registered the error, and need to do so
 		// - if cred is false, and the message is there, we can just return nil to the sdk, which "true" for the boolean return value indicates;
@@ -117,8 +120,11 @@ func (h *Handler) WaitingForCredential(cfg *v1.Config) (bool, bool) {
 		h.processError(cfg, v1.ImportCredentialsExist, corev1.ConditionFalse, err, "%v")
 		return true, true
 	}
+	if !cfg.ConditionTrue(v1.ImportCredentialsExist) {
+		h.GoodConditionUpdate(cfg, corev1.ConditionTrue, v1.ImportCredentialsExist)
+	}
 
-	// this is either centos, or the cluster admin is using their own registry for rhel content, so we do not
+	// the credentials are already in place, or the cluster admin is using their own registry for rhel content, so we do not
 	// enforce the need for the credential
 	return false, false
 }
@@ -136,16 +142,9 @@ func (h *Handler) processSecretEvent(cfg *v1.Config, dockercfgSecret *corev1.Sec
 	removedState := false
 	switch cfg.Spec.ManagementState {
 	case operatorsv1api.Removed:
-		// so our current recipe to switch to rhel is to
-		// - mark mgmt state removed
-		// - after that complete, edit again, mark install type to rhel and mgmt state to managed
-		// but what about the secret needed for rhel ... do we force the user to create the secret
-		// while still in managed/centos state?  Even with that, the "removed" action removes the
-		// secret the operator creates in the openshift namespace since it was owned/created by
-		// the operator
 		// So we allow the processing of the secret event while in removed state to
-		// facilitate the switch from centos to rhel, as necessitating use of removed as the means for
-		// changing from centos to rhel since  we allow changing the distribution once the samples have initially been created
+		// facilitate the imagestreams like cli, must-gather, that are installed from the
+		// payload via this operator's manifest, but are not managed by this operator
 		logrus.Printf("processing secret watch event while in Removed state; deletion event: %v", event.Deleted)
 		removedState = true
 	case operatorsv1api.Unmanaged:

test/e2e/cluster_samples_operator_test.go

@@ -177,8 +177,7 @@ func verifyConditionsCompleteSamplesRemoved(t *testing.T) error {
 			return false, nil
 		}
 		if cfg.ConditionFalse(samplesapi.SamplesExist) &&
-			cfg.ConditionFalse(samplesapi.ImageChangesInProgress) &&
-			cfg.ConditionFalse(samplesapi.ImportCredentialsExist) {
+			cfg.ConditionFalse(samplesapi.ImageChangesInProgress) {
 			return true, nil
 		}
 

test/e2e/main_test.go

@@ -20,7 +20,9 @@ import (
 	"testing"
 	"time"
 
+	configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
 	sampopclient "github.com/openshift/cluster-samples-operator/pkg/client"
+	"github.com/openshift/cluster-samples-operator/test/framework"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
 	kubeset "k8s.io/client-go/kubernetes"
@@ -46,6 +48,17 @@ func TestMain(m *testing.M) {
 		fmt.Println("failed waiting for operator to start")
 		os.Exit(1)
 	}
+	opClient, err := configv1client.NewForConfig(kubeconfig)
+	if err != nil {
+		fmt.Printf("problem getting operator client %#v", err)
+		os.Exit(1)
+	}
+	err = framework.DisableCVOForOperator(opClient)
+	if err != nil {
+		fmt.Printf("problem disabling operator deployment in CVO %#v", err)
+		os.Exit(1)
+	}
+
 	os.Exit(m.Run())
 }
 

test/framework/cvo.go

@@ -0,0 +1,50 @@
+package framework
+
+import (
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	configv1 "github.com/openshift/api/config/v1"
+	configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
+)
+
+const (
+	clusterVersionName = "version"
+)
+
+func addCompomentOverride(overrides []configv1.ComponentOverride, override configv1.ComponentOverride) ([]configv1.ComponentOverride, bool) {
+	for i, o := range overrides {
+		if o.Group == override.Group && o.Kind == override.Kind &&
+			o.Namespace == override.Namespace && o.Name == override.Name {
+			if overrides[i].Unmanaged == override.Unmanaged {
+				return overrides, false
+			}
+			overrides[i].Unmanaged = override.Unmanaged
+			return overrides, true
+		}
+	}
+	return append(overrides, override), true
+}
+
+// DisableCVOForOperator disables the samples operator deployment so we can modify the version env
+func DisableCVOForOperator(operatorClient *configv1client.ConfigV1Client) error {
+	cv, err := operatorClient.ClusterVersions().Get(clusterVersionName, metav1.GetOptions{})
+	if err != nil {
+		return err
+	}
+
+	var changed bool
+	cv.Spec.Overrides, changed = addCompomentOverride(cv.Spec.Overrides, configv1.ComponentOverride{
+		Kind:      "Deployment",
+		Namespace: "openshift-cluster-samples-operator",
+		Name:      "cluster-samples-operator",
+		Unmanaged: true,
+	})
+	if !changed {
+		return nil
+	}
+	if _, err := operatorClient.ClusterVersions().Update(cv); err != nil {
+		return err
+	}
+
+	return nil
+}