Add e2e test for console operator Unmanaged and Removed states #112

jhadvig · 2019-01-21T15:56:58Z

Adding Unmanaged and Removed test cases.
Each tests is testing several resources from the "openshift-console" namespace - Deployment/ConfigMap/Service/Route and their availability/unavailability for each operator state.

Also added logs.go that I've get from to get better logs.

jhadvig · 2019-01-22T14:19:12Z

/test e2e-aws

benjaminapetersen · 2019-01-22T15:05:48Z

pkg/testframework/console.go

@@ -0,0 +1,162 @@
+package testframework


Thinking about this now, it might be nice to split it up. Right now it seems to be all the funcs needed just to test management state. I'd prob rather separate out feature tests than keep adding to this one file:

/pkg |_ testframework |_ console |_ managementstate.go |_ otherfeature.go |_ yetanotherfeature.go |_ deployment |_ etc

Thoughts?

Makes sense to split it here, but Im not sure what the otherfeatures will be. The ones that pop to my head are, to start/stop the operator by scaling it up and down, but that can be on a /pkg/testframework/operator.go file.
Not sure what else at this state of the console-operator we should test.

Oh, we will probably test the top level console config, which has, for example, LogoutRedirect. Also the Operator config itself has Branding, DocumentationBaseURL, as well as a NodeSelector override.

Eventually we may support custom Route or perhaps a set of Routes (in discussion).

benjaminapetersen · 2019-01-22T15:07:51Z

pkg/testframework/framework.go

@@ -19,6 +23,19 @@ var (
 	AsyncOperationTimeout = 5 * time.Minute
 )

+// Logger is an interface to report events from tests. It is implemented by


I'm wondering if framework.go might grow into a kitchen sink of a file. Maybe we can break this up now & encourage future growth into separate files to keep it tidy down the road.

(I realize there isn't much here yet, so it could just be /framework/logging.go & framework/resourceavailabilty.go or something a bit like that.)

I see you have logs file below already. Nice. Was just thinking about DumpObject... hmm.

Yep thats a good question. But I think it mainly depends on how beefy we want our e2e tests for the console operator. If it should just test each operator state(maybe some extra tests, scaling operator down to 0 and back to 1), I wouldn't worry about the scale of the file :)

Probably related to the above comment, we have a set of other features to test, but uncertain how much it will grow over time (I dont expect tons).

benjaminapetersen · 2019-01-22T15:17:22Z

test/e2e/managed_test.go


 	consoleapi "github.com/openshift/console-operator/pkg/api"
 	"github.com/openshift/console-operator/pkg/testframework"
 )

+// TestManaged sets console-operator to Managed state. After that "openshift-console
+// deployment is deleted after which the deploymnet is tested for availability, together


deploymnet -> deployment

benjaminapetersen · 2019-01-22T15:18:04Z

test/e2e/managed_test.go

-		t.Fatal(err)
+
+	errChan := make(chan error)
+	go testframework.IsResourceAvailable(errChan, client, "ConfigMap")


Do we want to delete each of these resources & then check each availability?

No only the openshift-console deployment is deleted. Wasnt sure if we want to delete all, or just some, or only one.

Right, only the deployment is deleted, but it checks if each of the other resources are still there.

yes, thinking about it now, I probably would be better to do it the opposite way. Delete more resource (eg.Deployment, Route, ConfigMap). Then check for all the resources.

benjaminapetersen · 2019-01-22T15:19:24Z

pkg/testframework/framework.go

@@ -57,3 +74,61 @@ func DeleteCompletely(getObject func() (metav1.Object, error), deleteObject func
 		return obj.GetUID() != uid, nil
 	})
 }
+
+// IsResourceAvailable checks if tested resource is available(recreated by console-ioerator),


ioerator -> operator

benjaminapetersen · 2019-01-22T15:20:09Z

test/e2e/unmanaged_test.go

+
+	t.Logf("waiting to check if the operator has not recreate console deployment...")
+	errChan := make(chan error)
+	go testframework.IsResourceAvailable(errChan, client, "ConfigMap")


Should we be asking for a named resources? For example, we have 2 configmaps now.

(other resources are singletons, though there is also more than 1 secret in the namespace)

yeah, so here I wasn't sure if we want to nuke all the resources, or just some, or just one?
But since the operator should recreate any resource it should be enough to just test some resources. Thoughts ? :)

Agree, though it might be good to know for certain it is recreating all of the expected resources. If our listOpts funcs are ever updated, it could be possible the operator would stop paying attention to a resource it needs to manage. Currently the funcs are noop except for the OAuth client func.

benjaminapetersen · 2019-01-22T15:47:45Z

pkg/testframework/framework.go

+	errChan <- err
+}
+
+func getResource(client *Clientset, resource string) error {


Thinking about this:

// current unnamed getResource(client, 'ConfigMap') // vs named? getResource(client, 'configmap', 'console-config') // vs specific-to-client funcs? getConfigmapByName('console-config') getSecretByName('oauth-config')

Makes sense to have resource based getters 👍

Realized it doesn't actually get the resource, it returns only an error. So if you don't want to make specific ones, since this is more of a helper, it could be something like resourceExists()

benjaminapetersen · 2019-01-22T15:52:50Z

pkg/testframework/console.go

+	if err != nil {
+		DumpObject(logger, "the latest observed state of the console resource", cr)
+		DumpOperatorLogs(logger, client)
+		return fmt.Errorf("failed to wait to change console operator state to 'Removed': %s", err)


This branch is always operator state to 'Removed'?

nope, will update ! :)

benjaminapetersen · 2019-01-22T16:00:04Z

pkg/testframework/console.go

+
+type operatorStateReactionFn func(cr *v1alpha1.Console) bool
+
+func ensureConsoleIsInDesiredState(logger Logger, client *Clientset, state operatorsv1alpha1.ManagementState) error {


So ensureConsoleIsInDesiredStateleads me to think ALL console state, not just ManagementState. I would probably expect the world ensure does an UPDATE as well to set the state if it is not already correct, but it looks like the PATCH is in the functions wrapping this call.

Thinking about this.

yeah, probably the wording is not the best.
What about isConsoleInDesiredState || checkConsoleForDesiredState ?

or isManagementStateManaged(), isManagementStateRemoved()?

benjaminapetersen · 2019-01-22T16:00:29Z

pkg/testframework/console.go

+	_, err = client.ConsoleV1alpha1Interface.Consoles(consoleapi.OpenShiftConsoleOperatorNamespace).Patch(consoleapi.ResourceName, types.MergePatchType, []byte(`{"spec": {"managementState": "Managed"}}`))
+	if err != nil {
+		if errors.IsNotFound(err) {
+			return nil


We are ok here if errors.IsNotFound?

no we can get rid of that 👍

Yeah I bet we want it to fail in this case.

benjaminapetersen · 2019-01-23T16:39:13Z

pkg/testframework/framework.go

@@ -19,6 +23,19 @@ var (
 	AsyncOperationTimeout = 5 * time.Minute
 )

+// Logger is an interface to report events from tests. It is implemented by
+// testing.T.
+type Logger interface {


Should Logger interface be in the logs.go file?

Chewing on the logger. In the rest of the project, I've been using logrus.

Mostly just asking "what do we gain doing it differently here".

jhadvig · 2019-01-23T20:52:45Z

@benjaminapetersen updated the PR based on our conversation. PTAL :)

jhadvig · 2019-01-23T22:10:46Z

/test e2e-aws

jhadvig · 2019-01-24T12:15:09Z

/retest

jhadvig · 2019-01-24T13:11:31Z

/test e2e-aws

benjaminapetersen · 2019-01-24T21:24:04Z

/retest

jhadvig · 2019-01-24T22:42:09Z

Last e2e-aws failure

template pod "e2e-aws" failed: the pod ci-op-4hcvy3x2/e2e-aws failed after 43m26s (failed containers: setup, test): ContainerFailed one or more containers exited

Container setup exited with code 1, reason Error
---
level=warning msg="Found override for ReleaseImage. Please be warned, this is not advised"
level=info msg="Consuming \"Install Config\" from target directory"
level=info msg="Creating cluster..."
level=info msg="Waiting up to 30m0s for the Kubernetes API..."
level=fatal msg="waiting for Kubernetes API: context deadline exceeded"
---
Container test exited with code 1, reason Error
---
Another process 
---

jhadvig · 2019-01-24T22:42:34Z

/retest

jhadvig · 2019-01-25T08:33:35Z

/retest

jhadvig · 2019-01-25T09:58:29Z

@benjaminapetersen we are green :)

benjaminapetersen · 2019-01-25T20:45:43Z

/lgtm

awesome.

openshift-ci-robot · 2019-01-25T20:45:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benjaminapetersen, jhadvig

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [benjaminapetersen]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

benjaminapetersen · 2019-01-28T15:26:47Z

yay 🦄

openshift-ci-robot assigned benjaminapetersen Jan 21, 2019

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 21, 2019

openshift-ci-robot requested review from enj and spadgett January 21, 2019 15:57

benjaminapetersen reviewed Jan 22, 2019

View reviewed changes

benjaminapetersen reviewed Jan 23, 2019

View reviewed changes

jhadvig force-pushed the unmanaged branch from 24822ab to 47e612a Compare January 23, 2019 20:52

jhadvig force-pushed the unmanaged branch from 47e612a to d5a21c5 Compare January 23, 2019 20:53

Add e2e test for console operator Unmanaged and Removed states

a6dc09a

jhadvig force-pushed the unmanaged branch from d5a21c5 to a6dc09a Compare January 24, 2019 21:34

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 25, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 25, 2019

openshift-merge-robot merged commit 6f9d527 into openshift:master Jan 25, 2019


		type operatorStateReactionFn func(cr *v1alpha1.Console) bool

		func ensureConsoleIsInDesiredState(logger Logger, client *Clientset, state operatorsv1alpha1.ManagementState) error {

Add e2e test for console operator Unmanaged and Removed states #112

Add e2e test for console operator Unmanaged and Removed states #112

Conversation

jhadvig commented Jan 21, 2019

jhadvig commented Jan 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhadvig commented Jan 23, 2019

jhadvig commented Jan 23, 2019

jhadvig commented Jan 24, 2019

jhadvig commented Jan 24, 2019

benjaminapetersen commented Jan 24, 2019

jhadvig commented Jan 24, 2019 • edited Loading

jhadvig commented Jan 24, 2019

jhadvig commented Jan 25, 2019

jhadvig commented Jan 25, 2019

benjaminapetersen commented Jan 25, 2019

openshift-ci-robot commented Jan 25, 2019

benjaminapetersen commented Jan 28, 2019

jhadvig commented Jan 24, 2019 •

edited

Loading