Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add e2e test that deploys two Cass clusters and checks that Clusters … #16

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

burmanm
Copy link
Contributor

@burmanm burmanm commented Nov 25, 2020

…are re-registered. Closes #10

┆Issue is synchronized with this Jira Bug by Unito

@burmanm burmanm requested a review from jsanda November 25, 2020 14:15
@burmanm burmanm self-assigned this Nov 25, 2020
@burmanm
Copy link
Contributor Author

burmanm commented Nov 25, 2020

Last commit allows doing

[michael@nina reaper-operator]$ ORG=burmanm REG=localhost:5000 make docker-build docker-push e2e-test

And it runs the e2e-tests using the just built reaper-operator (if the kind for example would read from repo localhost:5000 like in their example)

@@ -82,6 +82,7 @@ vet:
PHONY: e2e-test
e2e-test:
@echo Running e2e tests
cd test/config/deploy_reaper_test && $(KUSTOMIZE) edit set image controller=${REV_IMAGE} && cd -
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been thinking about this since you starting working on reaper-operator and was about to create a ticket but decided to start reviewing this PR instead. I am glad I did :)

The problem with calling kustomize edit here is that it makes the change to the kustomization.yaml file which is stored in git. That turns into an ugly situation. Unfortunately kustomize build does not allow you to pass arguments. I need to look into this some more. multibases might help here.

@@ -0,0 +1,62 @@
apiVersion: cassandra.datastax.com/v1beta1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the only differences between cassdc-non-reaper.yaml and cassdc.yaml are the name and clusterName properties. We can avoid a lot of that duplication. I will work on an example of how best to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the size is different (again, to speed up the start and reduce resource usage). But yes, templating this would be a better option.

@@ -36,31 +38,21 @@ var (
var _ = Describe("Deploy Reaper with Cassandra backend", func() {
Context("When a Cassandra cluster is deployed", func() {
Specify("Reaper is deployed", func() {
// Speed up cassandradatacenter_controller status checks
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we initially discussed this scenario, I figured it would be a separate test. Do you think that would make more sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the reason I did in the main test was speed. Already the first test takes a lot of time to execute, so setting another with basically the same properties (other than one extra cluster) doubles the execution time. I'm not sure if there's value for two tests with only this change.

If we would get the cassdc startup to be faster, then adding multiple stages / tests would of course be better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the reason I did in the main test was speed.

I know that speed is an issue, but I still prefer a separate test mostly because it is a different scenario. For CI, we can run tests in parallel. We can also see what can be done to speed things up.

resources:
- cassdc.yaml
- cassdc-non-reaper.yaml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment below about creating a separate test. Assuming we do that we will want to create some new kustomize templates. We might create something like this under test/config/cassdc:

├── kustomization.yaml
├── bases
├── test-a
├── test-b
└── test-c

The bases directory would have the common CassandraDatacenter template. Each test- directory would extend the base.

Then under the test/config directory we might have:

├── cass-operator
├── cassdc
└── tests
    ├── test-a
    └── test-b

where each test- subdirectory would have its own kustomization.yaml file. I need to think about this some more.

@@ -36,31 +38,21 @@ var (
var _ = Describe("Deploy Reaper with Cassandra backend", func() {
Context("When a Cassandra cluster is deployed", func() {
Specify("Reaper is deployed", func() {
// Speed up cassandradatacenter_controller status checks
os.Setenv("REQUEUE_DELAY_STATUS_CHECK", "1m")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you aware, but we override the env vars, inlcuding REQUEUE_DELAY_STATUS_CHECK, with a kustomize patch.

Take a look at the output of kustomize build test/config/deploy_reaper_test to see.

This is done in test/config/deploy_reaper_test/reaper-operator-patch.yaml.

Copy link
Contributor

@jsanda jsanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@burmanm can you go ahead update from master since the kustomize changes for using a different operator image have been merged?

I will make a point to add some docs tomorrow. Here is the short version of what do to:

# Build and push your own image
$ ORG=burmanm make docker-build docker-push

# This will create test/config/deploy_reaper_test/overlays/forks/burmanm/kustomization.yaml
# We will need to update the script to generate the fork overlay dir
$ REGISTRY=docker.io OVERLAY=burmanm ORG=burmanm make init-test-overlays

# Run the test
$ TEST_OVERLAY=burmanm make e2e-test

@@ -36,31 +38,21 @@ var (
var _ = Describe("Deploy Reaper with Cassandra backend", func() {
Context("When a Cassandra cluster is deployed", func() {
Specify("Reaper is deployed", func() {
// Speed up cassandradatacenter_controller status checks
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the reason I did in the main test was speed.

I know that speed is an issue, but I still prefer a separate test mostly because it is a different scenario. For CI, we can run tests in parallel. We can also see what can be done to speed things up.

@burmanm
Copy link
Contributor Author

burmanm commented Dec 11, 2020

Would these changes make it more like you envisioned?

@@ -1,2 +1,8 @@
resources:
- base
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
images:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come you have this defined here?

})

Specify("More clusters are created", func() {
// Since we didn't clean the previous resources, we don't need to redeploy everything
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are tests guaranteed to execute in the order in which they are declared?

More importantly, this limits our ability to execute tests in parallel in a CI environment. cass-operator puts every test in a separate suite which makes it really easy to run them in parallel. I don't know if we need to do that but I figured separate test files and Describe block would facilitate running them in parallel.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case if I understood the documentation correctly they should:

"Ginkgo’s default behavior is to only permute the order of top-level containers – the specs within those containers continue to run in the order in which they are specified in the test file. "

// By default this function will run kustomize build on dir/overlays/k8ssandra. This will
// result in using upstream operator images. If you are testing against a fork, then set
// the TEST_OVERLAY environment variable to specify the fork overlay to use. When
// TEST_OVERLAY is set this function will run kustomize build on
// dir/overlays/forks/TEST_OVERLAY which will allow you to use a custom operator image.
func KustomizeAndApply(namespace, dir string) {
func KustomizeAndApply(namespace, dir, overlay string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come you added the overlay parameter here? The goal was to have a way for someone to run the tests against his own image. By adding the overlay parameter, I will have to modify the test to use a different image. And then I would have to do it for every test.

@adejanovski
Copy link
Contributor

@burmanm @jsanda, should we close this as won't do, or do we want to get this merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Add e2e test for registering CassandraDatacenters
4 participants