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

working setup of flink via kuttl #285

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

working setup of flink via kuttl #285

wants to merge 1 commit into from

Conversation

kensipe
Copy link
Member

@kensipe kensipe commented Jul 24, 2020

Signed-off-by: Ken Sipe [email protected]

Copy link
Member

@nfnt nfnt left a comment

Choose a reason for hiding this comment

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

Where is this being tested? Whouldn't we need to add ./repository/flink/docs/demo/tests to testDirs in kuttl-test.yaml?

status:
phase: Running

# confirms that kudo is running
Copy link
Member

Choose a reason for hiding this comment

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

Why is it necessary to check this here?

Copy link
Member Author

Choose a reason for hiding this comment

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

fail fast and portability
frankly if kudo is running this is a quick sub second check... what are the concerns?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's not a pattern we should promote: this is something that is covered beforeAll tests (by using kudo init --wait) option. I guess you could see it as very defensive programming but frankly, this shouldn't be necessary

kind: TestStep
commands:
- command: kubectl delete storageclass standard
- command: kubectl apply -f https://raw.githubusercontent.com/rancher/local-path-provisioner/master/deploy/local-path-storage.yaml
Copy link
Member

Choose a reason for hiding this comment

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

This is applying the same StorageClass that has just been deleted, see https://github.com/rancher/local-path-provisioner/blob/master/deploy/local-path-storage.yaml#L82. Why is this necessary to do here? This also means that the error condition above will always fire.

Copy link
Member

Choose a reason for hiding this comment

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

@kensipe I see this as an infra step. If we run these tests along with other tests present in this repository we might be changing the outcome of the tests. What about just using the storage class that is already present and which is being deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah... these tests are great for setting up and running / verifying the flink demo... perhaps we shouldn't hook them into the infra... is there any suggestion?

apiVersion: kuttl.dev/v1beta1
kind: TestStep
commands:
- command: kubectl kudo install zookeeper --operator-version=0.3.0 --skip-instance
Copy link
Member

Choose a reason for hiding this comment

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

Let's use local packages here, i.e. relative paths instead of packages from the community repository. This will allow us to see if (local) changes to any of these operators break this test.
Here and for kafka and flink below.

Copy link
Member Author

Choose a reason for hiding this comment

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

perhaps these tests are different... and should be relocated or not hooked into the infra... this is under the "demo" folder and these teams are similar to our use of kuttl for MWT.. they setup a cluster with the flink demo.

On the other hand... I could see value in having the demo checked on PRs to confirm it still works... hmm..

Copy link
Contributor

@zen-dog zen-dog left a comment

Choose a reason for hiding this comment

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

This PR is basically outdated: newest (but unmerged) flink-demo requires only this command to install:

$ kubectl kudo install repository/flink/docs/demo/financial-fraud/demo-operator --instance flink-demo

and none of the --skip-instance prerequisites. The only reason the above PR is not merged is that we needed to bump KUDO to 0.15 first

apiVersion: kuttl.dev/v1beta1
kind: TestStep
commands:
- command: kubectl kudo install zookeeper --operator-version=0.3.0 --skip-instance
Copy link
Contributor

Choose a reason for hiding this comment

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

None of these steps are necessary once #279 is merged

@nfnt
Copy link
Member

nfnt commented Oct 16, 2020

Let's update this to use a recent version of flink-demo that uses KUDO's dependency feature. With that, most setup steps aren't necessary anymore, the flink-demo operator will take care of installing it's dependencies. Furthermore, it isn't necessary to change anything regarding local storage in kind (as done in 01-update-storage-class.yaml), because this is part of a default cluster. IMO, if I'm not missing something, the test could be as simple as running kubectl kudo install ../../financial-fraud/demo-operator --instance flink-demo and waiting for the flink-demo Instance to complete. Checking that the dependencies have been installed is also a great test. So it's what currently done in step 3,4,5,6, without steps 1,2. Though all of these assert in 3,4,5,6 can be done in a single step, because they should all be true once the flink-demo instance is complete.

@nfnt
Copy link
Member

nfnt commented Oct 16, 2020

One thing might be a problem though: We already install O/OVs for a lot of operators as part of the test setup: https://github.com/kudobuilder/operators/blob/master/kuttl-test.yaml. We should run this test in a separate namespace to ensure that new O/OVs are installed for the flink-demo operator and its dependencies.

@kensipe
Copy link
Member Author

kensipe commented Oct 16, 2020

@nfnt unless kuttl is configured to run in a specific namespace... a new namespace is create for each test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants