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

enhancement(tests): Kubernetes E2E test framework #2702

Merged
merged 69 commits into from
Jul 29, 2020
Merged

Conversation

MOZGIII
Copy link
Contributor

@MOZGIII MOZGIII commented May 28, 2020

Will be rebased after #2653 is merged. 🎉

This PR adds E2E (end-to-end) test for our Kubernetes integration.

It also renames kubernetes-integration-tests into kubernetes-e2e-tests, and introduces a concept of an E2E test itself - a kind of test where we build a full Vector binary, deploy it and assert it's observable behavior. Very similar conceptually to the test harness, but it's part of the main Vector tree.

Check out CI jobs to see it in action, and take a look at updated CONRTIBUTING.md for how to run tests locally.

Closes #2632.

@github-actions
Copy link

github-actions bot commented May 28, 2020

Great PR! Please pay attention to the following items before merging:

Files matching Cargo.lock:

  • Has at least one other team member approved the dependency changes?

This is an automatically generated QA checklist based on modified files

@MOZGIII MOZGIII changed the title enhancement(tests): Kubernetes test framework enhancement(tests): Kubernetes E2E framework Jun 4, 2020
@MOZGIII MOZGIII changed the title enhancement(tests): Kubernetes E2E framework enhancement(tests): Kubernetes E2E test framework Jun 4, 2020
@MOZGIII MOZGIII force-pushed the k8s-test-framework branch 2 times, most recently from 1c96467 to 8c792a6 Compare June 4, 2020 20:34
@MOZGIII MOZGIII changed the base branch from master to k8s-impl June 4, 2020 20:47
@MOZGIII MOZGIII force-pushed the k8s-impl branch 3 times, most recently from aa1c6c7 to 8fdfa92 Compare June 8, 2020 21:39
@MOZGIII MOZGIII force-pushed the k8s-test-framework branch 2 times, most recently from 0a041fa to c4baf90 Compare June 9, 2020 23:48
@MOZGIII MOZGIII force-pushed the k8s-impl branch 2 times, most recently from a3a882e to 2fe4731 Compare June 10, 2020 20:34
@MOZGIII MOZGIII force-pushed the k8s-test-framework branch 4 times, most recently from 55827ad to 74ab186 Compare June 12, 2020 16:40
@MOZGIII MOZGIII force-pushed the k8s-test-framework branch 4 times, most recently from 7387876 to 5217151 Compare June 15, 2020 11:13
@MOZGIII MOZGIII force-pushed the k8s-impl branch 2 times, most recently from cb14221 to c0f2131 Compare June 15, 2020 15:17
Copy link
Contributor

@Hoverbear Hoverbear left a comment

Choose a reason for hiding this comment

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

It's absolutely not okay to try to push docker images to the docker hub as part of a e2e test suite.

make test-e2e-kubernetes CONTAINER_IMAGE_REPO=hoverbear/vector-test USE_MINIKUBE_CACHE=true

image

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Jul 24, 2020

You're hitting a minikube bug here: kubernetes/minikube#8799
Please use minikube v0.11.0 until it's resolved.

@Hoverbear Hoverbear self-requested a review July 24, 2020 20:02
@Hoverbear
Copy link
Contributor

Ok. :) Consider this unblocked then.

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Jul 25, 2020

Another issue: kubernetes/minikube#8840
Rolling back v1.16.12 to v1.16.13 for now.

@binarylogic
Copy link
Contributor

@MOZGIII just checking in. What's remaining here? @Hoverbear given your review, do you approve or still need changes made?

@Hoverbear
Copy link
Contributor

I'll see if minikube has been updated and try to rerun.

@Hoverbear
Copy link
Contributor

So I'd like make test-e2e-kubernetes to work like our other test commands and not require additional env vars to run. Could we make USE_MINIKUBE_CACHE=true default?

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Jul 28, 2020

minikube still at v1.12.1, so none of the issues were addressed.

Could we make USE_MINIKUBE_CACHE=true default?

We can autodetect it. See #2702 (comment)
I gave it some thought, and I think we autodetecting it isn't that bad. Defaulting it to just true is very surprising, imo.

However, the command will still not be really "usable" as the rest of the test commands: those are optimized for local run, while E2E is targeted towards optimizing the CI run use case - the reason for that is it's main goal is to test release builds, rather than debug. The overall theme of this test requiring manual involvement from the user remains, and it's sort of by design.

@Hoverbear
Copy link
Contributor

Okay so we need to wait for minikube to update before we merge?

It still seems like USE_MINIKUBE_CACHE is basically asking you to:

  • If true, use the local minihub docker registry
  • If false, push images to the docker hub.

Can you explain why you think it would be more surprising to a user that a test run is using their docker credentials to push images? As opposed to use a local cache on the minikube they'll be testing against?

@Hoverbear
Copy link
Contributor

I think ec3f9c5 solves the USE_MINIKUBE_CACHE issue :)

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Jul 28, 2020

I think everything's sorted out. Are we ready to merge?

  • If true, use the local minihub docker registry

It's slightly more complicated. minikube cache doesn't use local registry - it copies an image directly to the node it manages, and then it imports it and sets up a "lock" so that it's not GC'd by the kubelet. They ship their own custom system components into the kube-system ns to support it. So, even technically, no registry is involved.

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Jul 28, 2020

Okay so we need to wait for minikube to update before we merge?

I don't think it worth it. We can just keep using a working version. There are relevant TODOs in place, and issues at upstream are opened.

@Hoverbear
Copy link
Contributor

Ok!

@Hoverbear
Copy link
Contributor

So I noted as a ZFS user minikube can't work. So I had to use microk8s which does work very well if you do not have docker-ce from docker installed.

@Hoverbear
Copy link
Contributor

@MOZGIII I wonder if we can make QUICK_BUILD default for the test-e2e-kubernetes runs?

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Jul 28, 2020

@MOZGIII I wonder if we can make QUICK_BUILD default for the test-e2e-kubernetes runs?

It doesn't work on mac, and has some other limitations that make it poorly suitable to be the default.
We can work towards better E2E tests support in the future - once we have some issues sorted out we can make it the default.

@Hoverbear
Copy link
Contributor

Ok!

@MOZGIII MOZGIII merged commit 6dafbeb into master Jul 29, 2020
@MOZGIII MOZGIII deleted the k8s-test-framework branch August 3, 2020 14:32
mengesb pushed a commit to jacobbraaten/vector that referenced this pull request Dec 9, 2020
* Correct test-integration-kubernetes at Makefile

Signed-off-by: MOZGIII <[email protected]>

* Fix the tag overwrite logic at scripts/deploy-kubernetes-test.sh

Signed-off-by: MOZGIII <[email protected]>

* Make scripts/test-integration-kubernetes.sh more tweakable

Signed-off-by: MOZGIII <[email protected]>

* Reorder namespace and global config deletion command

The idea is namespace removal takes the longest, so we'd rather leave it
hanging than config deletion. Then is user gets tired of waiting and
sends a SIGINT we don't leave the global config dangling - just the
namespace removal, which will complete in the background.
So it's just a user experience improvement.

Signed-off-by: MOZGIII <[email protected]>

* Add kubernetes-test-framework

Signed-off-by: MOZGIII <[email protected]>

* Implement a first PoC kubernetes test

Signed-off-by: MOZGIII <[email protected]>

* K8s integration test is really an e2e test, rename accordingly

Signed-off-by: MOZGIII <[email protected]>

* Do not even publish container image at CI since we use "none" minikube driver

Signed-off-by: MOZGIII <[email protected]>

* Isolate kubernetes e2e tests via requried-features

Signed-off-by: MOZGIII <[email protected]>

* Add lock to the test framework

Signed-off-by: MOZGIII <[email protected]>

* Add some test cases to k8s e2e tests

Signed-off-by: MOZGIII <[email protected]>

* Add the ability to use quick debug builds in e2e tests

Useful to speed up the development cycles

Signed-off-by: MOZGIII <[email protected]>

* Use a single thread for test

Signed-off-by: MOZGIII <[email protected]>

* Made test framework async

Signed-off-by: MOZGIII <[email protected]>

* Allow specifying scope

Signed-off-by: MOZGIII <[email protected]>

* Correct arguments preparation for cargo test at scripts/test-e2e-kubernetes.sh

Signed-off-by: MOZGIII <[email protected]>

* Get rid of $(RUN) at test-e2e-kubernetes target at Makefile

Signed-off-by: MOZGIII <[email protected]>

* Set LOG at distribution/kubernetes/vector-namespaced.yaml

Signed-off-by: MOZGIII <[email protected]>

* Add a test to validate the pods are properly excluded

This tool a while to implement, and required that we make framework async.

Signed-off-by: MOZGIII <[email protected]>

* Fix a typo

Signed-off-by: MOZGIII <[email protected]>

* Add test to assert we properly collect logs from multiple namespaces

Signed-off-by: MOZGIII <[email protected]>

* Polish the test framework API

Signed-off-by: MOZGIII <[email protected]>

* Add E2E tests section to the contribution guide

Signed-off-by: MOZGIII <[email protected]>

* Kubernetes E2E tests are no longer experimental, should work consistently

Signed-off-by: MOZGIII <[email protected]>

* Add kubernetes version to the test name

Signed-off-by: MOZGIII <[email protected]>

* Bump minikube

Signed-off-by: MOZGIII <[email protected]>

* Bump kubernetes releases

Signed-off-by: MOZGIII <[email protected]>

* Use minikube cache instead of manually moving image around

Signed-off-by: MOZGIII <[email protected]>

* Test against multiple container runtimes

Signed-off-by: MOZGIII <[email protected]>

* Remove unused repeating_echo_cmd

Signed-off-by: MOZGIII <[email protected]>

* Display timeout

Signed-off-by: MOZGIII <[email protected]>

* Shorter title

Signed-off-by: MOZGIII <[email protected]>

* Switch to docker driver at minikube

Signed-off-by: MOZGIII <[email protected]>

* Remove the no_newline_at_eol test

Turns out, this test was invalid. The root cause with this is that, in
essence, Kubernetes expects logs to consist of line, with line being
defined as in POSIX - a sequence of characters *ending with \n*.
Thus it's *not valid* to emit a log line without the terminating newline
symbol in Kubernetes.
One effect of this is that when using the CRI log format, lines won't be
considered complete until we emit a newline character arrives - and the
additional content before the newline will be added to the log line that's
missing the newline.

Given all of the above, there's no reason for this test to exist. The
reason it was added was the behaviour detail of the docker log driver,
but it's a mere implementation detail, and it we should abstract from it.

The original statement of the test is also ill-posed, cause, as explained
above, it's non-partial messages (and, generally speaking, any message)
that doesn't end with newline isn't a valid log line in the first place.

Signed-off-by: MOZGIII <[email protected]>

* Increase timeout to rollout vector to 30s

Signed-off-by: MOZGIII <[email protected]>

* Temporarily disable crio

Signed-off-by: MOZGIII <[email protected]>

* Apply workaround for CRIO

Signed-off-by: MOZGIII <[email protected]>

* Fix clippy

Signed-off-by: MOZGIII <[email protected]>

* Unset log level in skaffold dev config to fallback to the one set in container

Signed-off-by: MOZGIII <[email protected]>

* Add exec_tail to the test framework

Signed-off-by: MOZGIII <[email protected]>

* Fix a typo at the comment

Signed-off-by: MOZGIII <[email protected]>

* Fix the typos and styling at the crate doccomment

Signed-off-by: MOZGIII <[email protected]>

* Bump k8s versions for E2E tests at CI

Signed-off-by: MOZGIII <[email protected]>

* Rename template params to pascal case

Signed-off-by: MOZGIII <[email protected]>

* Remove Drop from ResourceFile

Signed-off-by: MOZGIII <[email protected]>

* Proper authors

Signed-off-by: MOZGIII <[email protected]>

* Rename crate to k8s-test-framework

More in-line with the naming patterns of the rest of the k8s-related
crates.

Signed-off-by: MOZGIII <[email protected]>

* Correct kubectl comment at the interface

Signed-off-by: MOZGIII <[email protected]>

* Bumped k8s and minikube versions at CI

Signed-off-by: MOZGIII <[email protected]>

* Add a comment explaining the timeout at pod filtering test

Signed-off-by: MOZGIII <[email protected]>

* Rollback minikube to 0.11.0

Signed-off-by: MOZGIII <[email protected]>

* Update CONTRIBUTING.md

Co-authored-by: Ana Hobden <[email protected]>
Signed-off-by: MOZGIII <[email protected]>

* Update distribution/kubernetes/vector-namespaced.yaml

Co-authored-by: Ana Hobden <[email protected]>
Signed-off-by: MOZGIII <[email protected]>

* Fix an error at CONTRIBUTING.md

Signed-off-by: MOZGIII <[email protected]>

* Remove a trivial line from the doc

Signed-off-by: MOZGIII <[email protected]>

* Do second attemtp to start up minikube if the first one failed

Signed-off-by: MOZGIII <[email protected]>

* Print minikube logs if it fails to start

Signed-off-by: MOZGIII <[email protected]>

* Provide a default for CONTAINER_IMAGE_REPO if USE_MINIKUBE_CACHE is set

Signed-off-by: MOZGIII <[email protected]>

* Update the CONTRIBUTING.md for CONTAINER_IMAGE_REPO default if USE_MINIKUBE_CACHE is set

Signed-off-by: MOZGIII <[email protected]>

* Increase all rollout/wait timeouts to one minute

Signed-off-by: MOZGIII <[email protected]>

* Fix syntax error around minikube start command

Signed-off-by: MOZGIII <[email protected]>

* Rollback k8s v1.16.13 to v1.16.12 at CI

Signed-off-by: MOZGIII <[email protected]>

* Add minikube cache autodetection

Signed-off-by: MOZGIII <[email protected]>

* Document USE_MINIKUBE_CACHE=auto mode

Signed-off-by: MOZGIII <[email protected]>

* Add a note on minikube bug to CONTRIBUTING.md

Signed-off-by: MOZGIII <[email protected]>

* Add a note on minikube on ZFS to CONTRIBUTING.md

Signed-off-by: MOZGIII <[email protected]>

* Fix the doc comment at scripts/deploy-kubernetes-test.sh

Signed-off-by: MOZGIII <[email protected]>

* Apply a workaround for kubectl from snap

Signed-off-by: MOZGIII <[email protected]>

* Extract and reuse scripts/skaffold-dockerignore.sh

Signed-off-by: MOZGIII <[email protected]>

Co-authored-by: Ana Hobden <[email protected]>
Signed-off-by: Brian Menges <[email protected]>
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.

K8s: integration tests for the k8s source Kubernetes tests don't work locally
5 participants