Interrupting test suite leaves orphan etcd processes and data in /tmp

[invidian]

What steps did you take and what happened:

1. Run make test
2. Wait for it to start running tests.
3. Hit CTRL+C
4. Find garbage consuming RAM if you have /tmp mounted as tmpfs (common practice):

$ du -sh /tmp/k8s_test_framework_*
8.0K    /tmp/k8s_test_framework_047782320
8.0K    /tmp/k8s_test_framework_062909171
62M     /tmp/k8s_test_framework_101046869
62M     /tmp/k8s_test_framework_198215715
62M     /tmp/k8s_test_framework_263455972
62M     /tmp/k8s_test_framework_353449833
62M     /tmp/k8s_test_framework_547879397
8.0K    /tmp/k8s_test_framework_851006388
8.0K    /tmp/k8s_test_framework_971301376

5. Sometimes you can also find orphan etcd processes using pgrep etcd.

If you run test suite often or you only care about tests passing in a certain package while developing (Makefile currently does not allow specifying which tests to run, but for some packages you must run tests via Makefile to bring up the test suite up), I ended up few times with 10GB+ of data in /tmp consuming a lot of RAM and many etcd processes running consuming CPU.

What did you expect to happen:

Test suite ensures that resources are cleaned up after starting.

Environment:

• Cluster-api version: 046ab29
• Minikube/KIND version:
• Kubernetes version: (use kubectl version):
• OS (e.g. from /etc/os-release):

/kind bug
[One or more /area label. See https://github.com/kubernetes-sigs/cluster-api/labels?q=area for the list of labels]

[fabriziopandini]

At first sight, I'm not entirely sure this is entirely up to Cluster API, however, is someone have spare cycle to investigate this issue this could be beneficial to all the developers

/milestone Next
(setting milestone next because this is not related to a specific milestone)

/priority backlog
/help

[k8s-ci-robot]

@fabriziopandini:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

At first sight, I'm not entirely sure this is entirely up to Cluster API, however, is someone have spare cycle to investigate this issue this could be beneficial to all the developers

/milestone next
(setting milestone next because this is not related to a specific milestone)

/priority background
/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

@fabriziopandini: The label(s) priority/background cannot be applied, because the repository doesn't have them.

In response to this:

At first sight, I'm not entirely sure this is entirely up to Cluster API, however, is someone have spare cycle to investigate this issue this could be beneficial to all the developers

/milestone next
(setting milestone next because this is not related to a specific milestone)

/priority background
/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

@fabriziopandini: The provided milestone is not valid for this repository. Milestones in this repository: [Next, v0.3.x, v0.4.0, v0.4.x]

Use /milestone clear to clear the milestone.

In response to this:

At first sight, I'm not entirely sure this is entirely up to Cluster API, however, is someone have spare cycle to investigate this issue this could be beneficial to all the developers

/milestone next
(setting milestone next because this is not related to a specific milestone)

/priority background
/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[invidian]

At first sight, I'm not entirely sure this is entirely up to Cluster API, however, is someone have spare cycle to investigate this issue this could be beneficial to all the developers

If the test suite is managed someplace else and this is not an implementation bug, please point me where to report it instead :) I reported it here, as it occurred to me while working on this project.

[fabriziopandini]

There are several layers at play under CAPI, controller runtime, etcd and API Server binaries, tmpfs to name the most important, and CAPI has not direct control on the majority of those.
But until the problem is properly investigated, I guess it is fine to report here.

[sbueringer]

@invidian I think the problem right now is that nobody is cleaning up the testenv when the process is terminated via CTRL+C. During regular test run there are some clean up steps at the end. Maybe we can fix this by handling the signal we (probably) get from CTRL+C.

I assume the main question is if we can easily hook into the signal with the way we are executing the tests and if we are then able to find out what test environments are currently started and stop them gracefully.

I think I have the same issue when I run the tests, which are using testenv, via Intellij and then just terminate them in the middle.

Are you sure you really have to run some of the tests with the Makefile?

[invidian]

I found golang/go#41891 now, so it seems it's not trival to do in pure Go. Perhaps Makefile could have a target to trigger the cleanup post test-run then?

[sbueringer]

@invidian I'm wondering if we can implement a cleanup via a signal handler, somewhere around here:

cluster-api/controllers/suite_test.go

Lines 115 to 118 in 49ccbb8

	fmt.Println("Tearing down test suite")
	if err := testEnv.Stop(); err != nil {
	panic(fmt.Sprintf("Failed to stop envtest: %v", err))
	}

with something like:
https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/manager/signals/signal.go#L35-L42

We could maybe re-use the signal handler we already have in suite_test.go:

ctx     = ctrl.SetupSignalHandler()

I'm not entirely sure what the implications are and which testEnv leads to the problem in your case. But if we find a pattern how we can solve this we can implement it for all testEnvs.

[invidian]

I'm also thinking now if maybe etcd and kube-apiserver processes could run in containers to simplify the cleanup and make it more sane. Or maybe tests utilizing those could run outside of plain unit test suite? As sort of integration tests or something.

[sbueringer]

@invidian As far as I know that would be a major refactoring of either our entire test suites which are currently using envtest or envtest of controller-runtime itself.

I don't think that's realistic (and worth the effort). It would also introduce a dependency to Docker or a container engine in general which is probably not desirable.